[PATCH] D97665: [InstSimplify] Don't fold comparisons of non-inbounds GEPs

Wed Mar 3 13:17:55 PST 2021

nikic added a reviewer: aqjune.
nikic added a comment.

@nlopes Can you please take a look at the alive result? I don't think the transform is correct, but alive says it is. The result of the GEP is a pointer of value zero. It has different provenance from the null pointer, but `icmp eq` should be a pure value comparison, independent of provenance, right?

================
Comment at: llvm/lib/IR/ConstantFold.cpp:1837
+      // The foldings below assume the GEP to be inbounds.
+      // TODO: Some of the logic can be applied for non-inbounds GEPs as well.
+      if (!CE1GEP->isInBounds())
----------------
LemonBoy wrote:
> Are non-inbounds GEP common enough to implement this?
I'd generally say that we should handle inbounds precisely just for the sake of clarity -- however, after reviewing all the folds below, it looks like all of them either require inbounds, or can only work on GEPs for which we would infer inbounds anyway (e.g. the gep GV1, 0 icmp gep GV2, 0 case). Given that, the early out here should be fine, and I think you can drop the TODO as well.

================
Comment at: llvm/lib/IR/ConstantFold.cpp:1854
             // so the result is greater-than
             return isSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
         } else if (isa<ConstantPointerNull>(CE1Op0)) {
----------------
Unrelated to this patch, but all the `isSigned` handling in this code looks wrong to me. This is effectively saying that GlobalValues can only appear in the lower half of the address space and not in the upper (negative) half, which is not true.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97665