Use the types from byval/load/gep instead of the pointer element type.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Note: I added RUNs for enough tests that I felt confident I had good coverage, but I didn't do all of them. I'm not really sure that the way I handled typed/opaque CHECK results is a good strategy.
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | ||
---|---|---|
728–730 | we're slightly powering down this transform under opaque pointers right? since previously we'd treat different pointer types as different and not bail out in the recursive check as often. |
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | ||
---|---|---|
728–730 | I think it goes both ways -- yes, opaque pointers will look equal and be deemed recursive in cases that weren't before. On the other hand, the former code considered it recursive if the type had a self pointer at all, even if that's not used in the promotion. I'll see if I can craft test examples for both. |
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | ||
---|---|---|
728–730 | if we're going to support opaque pointers here, we should emulate what would happen with opaque pointers and bail out if we recursively see any pointer type. that way when we do the opaque pointer switch, this pass doesn't suddenly power down. I'd rather power it down beforehand and see if anybody complains |
Make the recursion check the same for typed or opaque pointers
The pointer type is no longer considered at all when checking for
recursive loads in arg promotion, so we have parity with or without
opaque pointers. The new test ArgumentPromotion/opaque-recursion.ll
shows what otherwise could have worked fine with typed pointers.
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | ||
---|---|---|
728–730 | I think this is addressed in the new update. |
This looks good to me. Regarding the tests, I'd prefer to not modify them in this way and have a separate test with opaque pointers instead, otherwise update_test_checks doesn't work, and that's always a PITA when making changes.
Also worth noting that ArgPromotion uses areFunctionArgsABICompatible(), which currently accesses pointer element types and will need a signature change.
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp | ||
---|---|---|
957 | Don't think you need the hasByValAttr() call here, getParamByValType() should already return nullptr. |
I looked into this a bit more closely, and unfortunately ArgPromotion will require significant additional work (close to a full rewrite) to support opaque pointers. The core problem is that ArgPromotion is currently based around GEP index structure. With opaque pointers, GEP index structure can be arbitrary. The transform will need to be changed to an offset-based implementation.
Ok, I see that you've gone ahead with that rewrite in D118685, so I'll abandon this change.
we're slightly powering down this transform under opaque pointers right? since previously we'd treat different pointer types as different and not bail out in the recursive check as often.
it'd be nice to have a test for this change