Skip to content
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

ReplacePointerBitcastPass introduces invalid IR with struct field of a struct #1270

Closed
BukeBeyond opened this issue Dec 10, 2023 · 3 comments
Labels

Comments

@BukeBeyond
Copy link

This one is quite complex! The original code is a recursive BiQuad filter running on custom vectors and parallel distributors, with 4 passes atomically synced in one kernel. Everything runs fine, and at top speed!
https://godbolt.org/z/ca81s5Yer

Clspv's ReplacePointerBitcastPass gets confused when a new field of "struct.beyond::SS" type is introduced to the custom image "class.beyond::Samples2D".

%"class.beyond::A" = type { i64 }
%"class.beyond::Samples2D" = type <{ %"class.beyond::A", %"struct.beyond::Vector2D", %"struct.beyond::Vector2D.0", %"struct.beyond::Vector2D.0", %"struct.beyond::SS", [4 x i8] }>
%"struct.beyond::Vector2D" = type { i32, i32 }
%"struct.beyond::Vector2D.0" = type { float, float }
%"struct.beyond::SS" = type { i32 }

Now this field is not yet used anywhere in the code, but its mere existence triggers ReplacePointerBitcastPass to emit invalid bitcasts;

Invalid bitcast
%5185 = bitcast <2 x %"struct.beyond::SS"> %5184 to i64

https://godbolt.org/z/5e1EKT3cM

This is emitted from ReplacePointerBitcastPass.cpp ReduceType() if (!IsGEPUser) {...}

And subsequently Clspv crashes; "LLVM ERROR: Broken module found, compilation aborted!"

Strangely, the problem goes away introducing an i8 field at the end of "class.beyond::Samples2D" with automatic padding;

%"class.beyond::Samples2D" = type <{ %"class.beyond::A", %"struct.beyond::Vector2D", %"struct.beyond::Vector2D.0", %"struct.beyond::Vector2D.0", %"struct.beyond::SS", i8, [3 x i8] }>

https://godbolt.org/z/9Kjfb81Pc

another location works too, and no padding;

%"class.beyond::Samples2D" = type { %"class.beyond::A", %"struct.beyond::Vector2D", %"struct.beyond::Vector2D.0", %"struct.beyond::Vector2D.0", i8, %"struct.beyond::SS" }

https://godbolt.org/z/P96TbETKr

Also works, adding a second field inside SS;
%"struct.beyond::SS" = type { i32, i32 }
https://godbolt.org/z/M99b8P9vs

but not 3 fields, which Clspv compiles, but it crashes Vulkan;
%"struct.beyond::SS" = type { i32, i32, i32 }
https://godbolt.org/z/dYK1KM8qE

but 4 fields works again;
%"struct.beyond::SS" = type { i32, i32, i32, i32 }
https://godbolt.org/z/9M6a1edoz

Again, none of these fields, or the struct SS are yet used in the program, yet they bring out the dormant bugs from ReplacePointerBitcastPass.

I hope you like a good challenge! Good luck!

@dneto0 dneto0 added the bug label Dec 20, 2023
@rjodinchr
Copy link
Collaborator

This first godbolt link is compiling fine now. Should we close this issue?

@rjodinchr
Copy link
Collaborator

Feel free to reopen if I am missing something

@BukeBeyond
Copy link
Author

Well, the first one was always compiling as a baseline. It is the second one crashing Clspv, and the second from the last one that was crashing Vulkan.

After we applied the i8 Canonicalization of GEPs in our custom branch, Clang and LLVM became able to inline and eliminate more of these structure definitions which were triggering bugs in ReplacePointerBitcastPass.

A bit more on that here: #1292 (comment)

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

No branches or pull requests

3 participants