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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:454
+  if (RHS->getType() != PtrIdxTy)
+    RHS = Builder.CreateSExtOrTrunc(RHS, PtrIdxTy);
   if (IndexedSize != ElementSize) {
----------------
krzysz00 wrote:
> foad wrote:
> > Not necessarily related to your patch, but the "OrTrunc" makes me nervous (here and in a couple of other places in this patch). My understanding of the index type in datalayout is that it is a preferred type to use GEP indexes, but shouldn't actually have any semantic effect, so it would be wrong to truncate an index to the index type unless you can prove that it won't change the value.
> That's not my understanding of that field. The index field is maximal size of an offset into a pointer in address space N. For example, CHERI's capability pointers are (to my understanding) `128:128:128:64` meaning that the pointer value is 128 bits long, but only 64 bits of offset can be applied to it.
> 
> That is, in C terms, the pointer type is `intptr_t` and the index field is `size_t`. This patch fixes up a bunch of code that normalized pointer offsets to `intptr_t` instead of `size_t`, since LLVM historically assumed they were the same.
This is correct; see SelectionDAGBuilder::visitGetElementPtr for example where the use of the index type is explicit (and there's some commentary, too). It is legal to use an oversized type in the IR, but it will eventually be legalised. Undersized types are implicitly sign-extended.


================
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);
----------------
krzysz00 wrote:
> 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.
Hm, weirdly I don't see any changes to this function in CHERI LLVM, which makes me wonder how we don't hit issues here on CHERI-RISC-V given the RISC-V backend doesn't return true for STI->useAA()...


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:794
 
 bool SeparateConstOffsetFromGEP::canonicalizeArrayIndicesToPointerSize(
     GetElementPtrInst *GEP) {
----------------
Arguably this is now a slight misnomer


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:921
 
+/// Note: This only works if intptr_t == size_t
 void
----------------
IR is not C, plus the assert documents that anyway?


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:927
   Type *IntPtrTy = DL->getIntPtrType(Variadic->getType());
+  assert(DL->getPointerTypeSizeInBits(Variadic->getType()) ==
+             DL->getIndexTypeSizeInBits(Variadic->getType()) &&
----------------
Why not assert they're the same type?


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1066
     // As currently BasicAA does not analyze ptrtoint/inttoptr, do not lower to
     // arithmetic operations if the target uses alias analysis in codegen.
+    bool IsFatPointer = DL->getPointerSizeInBits(AddrSpace) !=
----------------
Comment probably needs updating to match


================
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);
----------------
Why is AddrSpace before the comment but IsFatPointer after?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143437