[PATCH] D142387: [SCCP] Use range info to prove AddInst has NUW flag.

Wed Jan 25 10:10:11 PST 2023

fhahn marked 2 inline comments as done.
fhahn added a comment.

Thanks for the reports and revert! Should be fixed in the recommitted version.

================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:191
+    auto RangeB = GetRange(Inst.getOperand(1));
+    auto NUWRange = ConstantRange::makeGuaranteedNoWrapRegion(
+        Instruction::Add, RangeB, OverflowingBinaryOperator::NoUnsignedWrap);
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > This should be guarded under `if (!Inst.hasNoUnsignedWrap())`. Otherwise we will report an unnecessary change.
> > Thanks, changed to  `if (!Inst.hasNoUnsignedWrap() &&NUWRange.contains(RangeA))`. Could also have a separate check guarding `makeGuaranteedNoWrapRegion`, which would increase nesting though and hopefully existing compilers should be smart enough to delay the computation :)
> > 
> Existing compilers almost certainly can't delay that computation (it's not LLVM_READONLY, and I'm not sure clang would sink the call even then).
Fair point, I was assuming it at least to get inlined, but then realized that it is not even defined in the header....

Updated in the recommitted version.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142387