[PATCH] D110245: [ConstantFolding] Fold ptrtoint(gep i8 null, x) -> x

Thu Sep 23 02:27:52 PDT 2021

arichardson marked 3 inline comments as done.
arichardson added inline comments.

================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1353-1355
+        // zext/trunc the inttoptr to pointer size.
+        FoldedValue = ConstantExpr::getIntegerCast(
+            CE->getOperand(0), DL.getIntPtrType(CE->getType()), false);
----------------
lebedev.ri wrote:
> arichardson wrote:
> > lebedev.ri wrote:
> > > I don't think this is correct.
> > > In `int -> ptr -> int`, the width can be different at all three stages,
> > > so i'd expect two sextOrTruncOrSelf casts back to back.
> > I believe the current code manually does a trunc if the `inttoptr` input type is larger than pointer size:
> > with a `p:64:64:64:32` datalayout, `ptrtoint (inttoptr i256 -1 to i8*) to i128` should result in
> > `trunc ((i256)-1 & UINT64_MAX) to i128`. The new code now does `zext (trunc i256 -1 to i64) to i128` which I believe yields the same value? No existing tests were affected by this refactoring so it's either equivalent or we are missing test coverage.
> > 
> > Slightly unrelated: For our (CHERI) usecase pointer size in bits is wrong since we have 128-bit pointers but only  64-bit address range. I would like to change it to index size, but I am not sure if that is acceptable upstream. If not I could add another datalayout field for pointers that specifies the address range (defaulting to index size). For now this works for us since we have patched DL.getIntPtrType() to return i64 for our addrspace(200) pointers, but I would prefer to be explicit whether we care about address range or pointer type size. This might also help with non-integral pointers that have additional metadata (e.g. bounds) like our CHERI capabilities.
> Ah, the reply i should have gotten is: "what are you talking about? there is a second cast, later on"
If this is not clear enough I can add a comment?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110245