Page MenuHomePhabricator

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

Authored by luqmana on Mar 30 2019, 2:38 PM.

Details

Summary

Teach CorrelatedValuePropagation to also handle sub instructions in addition to add. Relatively simple since makeGuaranteedNoWrapRegion already understood sub instructions. Only subtle change is which range is passed as "Other" to that function, since sub isn't commutative.

Note that CorrelatedValuePropagation::processAddSub is still hidden behind a default-off flag as IndVarSimplify hasn't yet been fixed to strip the added nsw/nuw flags and causes a miscompile. (PR31181)

Diff Detail

Repository
rL LLVM

Event Timeline

luqmana created this revision.Mar 30 2019, 2:38 PM
nikic added a comment.Apr 4 2019, 12:43 AM

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.

llvm/test/Transforms/CorrelatedValuePropagation/sub.ll
11 ↗(On Diff #192992)

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.

luqmana updated this revision to Diff 195963.Apr 19 2019, 11:11 PM

Rename option and method to be more general.

luqmana marked an inline comment as done.Apr 19 2019, 11:12 PM
luqmana added inline comments.
llvm/test/Transforms/CorrelatedValuePropagation/sub.ll
11 ↗(On Diff #192992)

Yea, mostly just to test that the right flags get added.

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.

PR31181 still repros for me:

With the nowrap flag marking disabled (default):

➜  llc -o PR31181.opt.s PR31181.opt.bc
➜  opt -instcombine -early-cse -jump-threading -correlated-propagation -loop-rotate -indvars -gvn -o PR31181.opt.bc PR31181.bc
➜  llc -o PR31181.opt.s PR31181.opt.bc                                                                                        
➜  gcc -o PR31181 PR31181.opt.s
➜  timeout -s 9 10 ./PR31181

Enabling nowrap flag marking:

➜  opt -instcombine -early-cse -jump-threading -correlated-propagation -loop-rotate -indvars -gvn -cvp-dont-add-nowrap-flags=false -o PR31181.opt.bc PR31181.bc
➜  llc -o PR31181.opt.s PR31181.opt.bc
➜  gcc -o PR31181 PR31181.opt.s
➜  timeout -s 9 10 ./PR31181
[1]    5928 killed     timeout -s 9 10 ./PR31181

I've committed the baseline sub.ll tests in rL358808. Could you please rebase over them and rerun update_test_checks.py?

luqmana updated this revision to Diff 195964.Apr 20 2019, 3:40 AM

Rebase over committed baseline tests.

nikic accepted this revision.Apr 20 2019, 4:07 AM

LGTM

This revision is now accepted and ready to land.Apr 20 2019, 4:07 AM
This revision was automatically updated to reflect the committed changes.