[PATCH] D60036: [CorrelatedValuePropagation] Mark subs that we know not to wrap with nuw/nsw.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Generally looks fine to me, but I'm not particularly familiar with CVP. I'd suggest to decouple the naming from add/sub in particular, as this optimization could also be easily extended to muls as well, which also support calculation of the nowrap region. So rename AddSubOp to just Op or BinOp, processAdd to processNoWrapFlags or so and the option to cvp-dont-add-nowrap-flags.

We should probably give reenabling the option by default another try (in a separate revision), at least I can't seem to reproduce the issue in PR31181 anymore.



================
Comment at: llvm/test/Transforms/CorrelatedValuePropagation/sub.ll:11
+; CHECK: %sub = sub nuw nsw i32 %a, 1
+  %sub = sub i32 %a, 1
+  br label %exit
----------------
These cases aren't particularly meaningful, because this is non-canonical IR -- usually the `sub %a, 1` gets canonicalized to `add %a, -1`, at which point the existing `add` code would add nowrap flags. Sub nowrap flags are generally only useful if the second operand is non-constant.

But I guess that for the purposes of this test this doesn't really matter, as it shows the relevant transform, even if the specific case can't occur naturally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60036