[PATCH] D130729: [SeparateConstOffsetFromGEP] [AMDGPU] Check legality for all uses of transformed GEP

Wed Aug 3 15:35:11 PDT 2022

jrbyrnes marked an inline comment as done.
jrbyrnes added a comment.

Hey Matt, Jay,

Thanks for the comments -- as always, they're very helpful and much appreciated.

Jay --

I precommited the test case via e0b16aaaf997 <https://reviews.llvm.org/rGe0b16aaaf99784efbb9dd9624787554d5584701a>. As you can see, we are producing illegal offsets for the flat_atomic_fadd. This is due to SeparateConstOffset pass modifying the GEP s.t. the offset is negative, which gets translated close to the 16bit unsigned max.

I believe all GEPs are technically legal at this stage, however, negative offsets for FLAT addresses are not supported / legal. Thus, if we produce an addrspace(0) address with a negative offset, we will need to handle it at some point or another. The approach here mimics some existing code in the SeparateConstOffset pass by invoking TTI->isLegalAddressingMode and simply disallows producing such an address.

Matt --

Thanks for pointing out that pass -- it does seem like a more appropriate place for this to be handled. I was able to hack together a solution for this using your approach, but I'll need to spend a bit more time to clean things up a bit. The benefit is we will still be able to perform the SeparateConstOffset optimization.

================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:915-916
+
+    if (!InterestingCase)
+      AllUsesValid &= resolvePtrToInt(Inst, AccumulativeByteOffset, Visited);
+
----------------
arsenm wrote:
> Doing anything to ptrtoint/inttoptr is probably wrong
Yes probably. If I were to continue with this approach, I would override & use the Analysis/PtrUseVisitor.h class (as it's already doing exactly what I want), which, by default, flags PtrToInts as escaped.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130729/new/

https://reviews.llvm.org/D130729