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

[Krzysztof Drewniak via Phabricator via llvm-commits]

krzysz00 marked an inline comment as done.
krzysz00 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:282-286
   if (!GEP->isInBounds()) {
-    Type *IntPtrTy = DL.getIntPtrType(GEP->getType());
-    unsigned PtrSize = IntPtrTy->getIntegerBitWidth();
+    Type *PtrIdxTy = DL.getIndexType(GEP->getType());
+    unsigned PtrSize = PtrIdxTy->getIntegerBitWidth();
     if (Idx->getType()->getPrimitiveSizeInBits().getFixedValue() > PtrSize)
+      Idx = Builder.CreateTrunc(Idx, PtrIdxTy);
----------------
arsenm wrote:
> Don't think this one is tested
You're right, added a test.


================
Comment at: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp:414
+  IntegerType *OffsetTy = DL->getIndexType(*Ctx, GVPtrTy->getAddressSpace());
+  APInt Offset(DL->getTypeSizeInBits(OffsetTy), /*val*/ 0, /*isSigned*/ true);
   auto *GEPO = cast<GEPOperator>(ConstExpr);
----------------
arsenm wrote:
> Don't think this one is tested
> 
GEPOperator::accumulateConstantOffset asserts that its input bitwidth has bitwidth equal to the index size of the GEP base, so this was a straight-up bug.


================
Comment at: llvm/lib/Transforms/Scalar/MergeICmps.cpp:160
 
-  APInt Offset = APInt(DL.getPointerTypeSizeInBits(Addr->getType()), 0);
+  APInt Offset = APInt(DL.getIndexTypeSizeInBits(Addr->getType()), 0);
   Value *Base = Addr;
----------------
arsenm wrote:
> Don't think this one is tested
> 
Same thing with accumulateConstantOffset 


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:927
 
-  Value *ResultPtr = Builder.CreatePtrToInt(Variadic->getOperand(0), IntPtrTy);
+  Value *ResultPtr = Builder.CreatePtrToInt(Variadic->getOperand(0), PtrIdxTy);
   gep_type_iterator GTI = gep_type_begin(*Variadic);
----------------
arsenm wrote:
> Doesn't creating a ptrtoint from a non-integral pointer break horribly?
Good catch, I've added guards to make sure non-integral address spaces don't get sent this direction. And that fat pointers don't enter this code path either.


================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:719
+  unsigned OffsetBitWidth = DL.getIndexSizeInBits(ASL);
+  APInt OffsetL(OffsetBitWidth, 0), OffsetR(OffsetBitWidth, 0);
   if (GEPL->accumulateConstantOffset(DL, OffsetL) &&
----------------
arsenm wrote:
> Don't think this one is tested
> 
Same note re accumulateConstantOffset()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143437