Skip to content

[CIR][ThroughMLIR] Lower structs and GetMemberOp #1565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

terapines-osc-2
Copy link
Contributor

Structs are implemented as memref<size x i8>. It is not feasible to represent them as tuples, for tuples can't be put in memref (i.e. pointers to structs would break if we did).

We use memref::ViewOp for this. Unlike PtrStrideOp, the reinterpret cast operation doesn't work here, as the result type is potentially different from i8.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. @keryell might have some input here

@bcardosolopes
Copy link
Member

Looks like some tests are failing

@terapines-osc-2
Copy link
Contributor Author

terapines-osc-2 commented Apr 17, 2025

Seems the failed tests are because of the rename of StructType. Now it's fixed.

@bcardosolopes
Copy link
Member

now that I landed your other PR, gh might automatically run the tests, gave it another click!

@keryell
Copy link
Collaborator

keryell commented Apr 17, 2025

Yes you can do that.
But there are some philosophical questions about what is the meaning of MLIR standard dialect lowering, along some points discussed in #1219
There is a trade-off between lowering to MLIR standard dialect as is today whatever low-level the generated code is and waiting for MLIR standard dialect to improve so that the code can be high-level.
Probably your approach of having something upstream today is good compared to what I have tried with "what if we had a better tuple in MLIR standard dialect in the future" in #1334

@keryell
Copy link
Collaborator

keryell commented Apr 17, 2025

In #1334 I use a tuple-like type to keep the high-level data structure and being able to have for example struct of struct or struct of arrays which have a value semantics which cannot be represented with memref but with the same trick as you to emulate GetMemberOp.

@bcardosolopes
Copy link
Member

Fails on windows for some reason!

@terapines-osc-2
Copy link
Contributor Author

It seems #1569 calculates wrong offsets for struct types, so the test cases will fail. For a non-packed struct { char a; int b; }, it thinks the offset of b is 1, but it should be 4. Currently CIR doesn't have test cases for these, so it's left unnoticed.
I'll try to solve it in another PR and leave this open for a while.

Still don't know why there's failure on windows though.

@bcardosolopes
Copy link
Member

I'll try to solve it in another PR and leave this open for a while.

Great!

Still don't know why there's failure on windows though.

Seems to be failing all across the board

@keryell
Copy link
Collaborator

keryell commented Apr 21, 2025

To rebase?

Structs are implemented as `memref<size x i8>`. It is not feasible to
represent them as tuples, for tuples can't be put in memref (i.e.
pointers to structs would break if we did).

We use `memref::ViewOp` for this. Unlike `PtrStrideOp`, the reinterpret
cast operation doesn't work here, as the result type is potentially
different from i8.
@terapines-osc-2
Copy link
Contributor Author

Now the struct offset issue is fixed.
As it's hard to write an independent test case for struct types, I choose to include the change here (it's small anyway).

@bcardosolopes
Copy link
Member

One alternative is to build your clang with ASAN enabled by the host compiler (using CMAKE's -DLLVM_USE_SANITIZER="Address"), that usually helps spotting windows only crashes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants