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

LemonBoy via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 4 01:45:41 PST 2021


LemonBoy added a comment.

> But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
> It would be great if I can see the input that causes miscompilation.

As stated here <https://github.com/AliveToolkit/alive2/issues/684#issuecomment-788122580> the gep is being generated by the SCEV pass.
A minimal test case that shows the gep being created (but not this miscompilation) is here <https://gist.github.com/LemonBoy/203846aac164a7e4dba92d9081898887>.



================
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())
----------------
nikic wrote:
> 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.
Some trivial geps can still be folded such as all-zero ones vs constant or unsigned comparisons with zero. But yes, I'll drop the TODO as those cases are pretty rare.


================
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)) {
----------------
nikic wrote:
> 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.
Yes, that looks wrong. I guess this is unlikely to cause problems as most (every?) pointer comparisons against null are of eq/neq type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97665



More information about the llvm-commits mailing list