[PATCH] D140844: [InstCombine] Canonicalize gep of addrspacecast

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2473
+      return replaceInstUsesWith(GEP, NewAC);
+    }
+  }
----------------
arsenm wrote:
> nikic wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > We probably shouldn't be reassociating if either address space is non-integral. I think that was one of the fixes that never happened for the existing code
> > > But yes,  moving the addrspacecast up makes more sense 
> > > We probably shouldn't be reassociating if either address space is non-integral.
> > 
> > Hm, why? I don't think we really consider GEP to operate on the integer representation?
> > 
> > > But yes, moving the addrspacecast up makes more sense
> > 
> > I think in that case I'll probably just ignore this discrepancy for now (and update tests with current opaque pointer behavior) and then return to this once typed pointers are removed and we don't have to worry about opposing transforms.
> The non-integral GEP could have some special clamping or wrapping behavior in one address space and not the other 
> The non-integral GEP could have some special clamping or wrapping behavior in one address space and not the other

Hm, is that legal? That doesn't really sound compatible with how we deal with GEP offsets in BasicAA (and stripAndAccumulateConstantOffsets for that matter). We treat offset operations in different address spaces as interchangeable, apart from trunc/sext.


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

https://reviews.llvm.org/D140844