[PATCH] D143437: [llvm] Use pointer index type for more GEP offsets (pre-codegen)

[Krzysztof Drewniak via Phabricator via llvm-commits]

krzysz00 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:794
 
 bool SeparateConstOffsetFromGEP::canonicalizeArrayIndicesToPointerSize(
     GetElementPtrInst *GEP) {
----------------
jrtc27 wrote:
> Arguably this is now a slight misnomer
... yeah, I should fix that


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1067
     // arithmetic operations if the target uses alias analysis in codegen.
-    if (TTI.useAA())
+    bool IsFatPointer = DL->getPointerSizeInBits(AddrSpace) !=
+                        DL->getIndexSizeInBits(AddrSpace);
----------------
jrtc27 wrote:
> arsenm wrote:
> > jrtc27 wrote:
> > > Why is AddrSpace before the comment but IsFatPointer after?
> > Don't know what FatPointer is meaning here, but isNonIntegralAddressSpace should cover it?
> Non-integral is overly strong, it also implies instability of the value if converted to an integer multiple times, which is not true of CHERI, nor AMDGPU AFAIK
Yeah, this is safeguarding against the possibility of having the buffer descriptor be an integral value, where you need to be very careful when adding things (to avoid carries into the metadata).


================
Comment at: llvm/lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:677
   case Candidate::GEP:
     {
+    Type *OffsetTy = DL->getIndexType(C.Ins->getType());
----------------
jrtc27 wrote:
> If you're changing the indentation below please also fix the curlies (and break) to match
I don't think this is changing the indentation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143437