[PATCH] D116332: [ConstantFold] Make icmp of gep fold offset based

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1257
+    // signed.
+    CmpInst::Predicate CmpPred = (CmpInst::Predicate)Predicate;
+    if (Ops0->getType()->isPointerTy() &&
----------------
Similar to your comment in D116168 - I'm not sure why we specify this parameter as `unsigned` instead of the actual enum. I see that we are not including any headers via ConstantFolding.h, so maybe there's some compile-time justification for it?


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1259
+    if (Ops0->getType()->isPointerTy() &&
+        (ICmpInst::isEquality(CmpPred) || ICmpInst::isUnsigned(CmpPred))) {
+      unsigned IndexWidth = DL.getIndexTypeSizeInBits(Ops0->getType());
----------------
`!ICmpInst::isSigned(CmpPred)` ?


================
Comment at: llvm/test/Transforms/InstSimplify/ConstProp/icmp-global.ll:201
 
 define i1 @global_gep_ugt_global_neg_offset() {
 ; CHECK-LABEL: @global_gep_ugt_global_neg_offset(
----------------
Add a comment for this test to explain that a 'true' result would be a miscompile (unlike the 'inbounds' variation above here)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116332