This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine][LV] Fold (add (zext (add X, -1)), 1) -> (zext X) if X is non-zero.
ClosedPublic

Authored by craig.topper on Jan 27 2023, 2:23 PM.

Details

Summary

This artifact can appear from the vectorizer. (add X, -1) is the
backedge taken count. It gets zero extended and then 1 is added to
it to get the trip count.

There is usually a dominating branch that rules out X being zero.

Alive: https://alive2.llvm.org/ce/z/NsRDwX

Alternatively, we could try to fix this in the vectorizer or the
SCEV expander maybe?

Still need to add InstCombine specific test, but hopefully the vectorizer tests serve as a demonstration.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 27 2023, 2:23 PM
craig.topper requested review of this revision.Jan 27 2023, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 2:23 PM
craig.topper edited the summary of this revision. (Show Details)Jan 27 2023, 2:26 PM

Should I proceed with this patch?

Should I proceed with this patch?

Seems ok to me. Add an alive link to the patch description:
https://alive2.llvm.org/ce/z/NsRDwX

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
979

FIXME -> TODO

craig.topper edited the summary of this revision. (Show Details)Jan 30 2023, 12:33 PM
nikic added inline comments.Jan 30 2023, 12:34 PM
llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
982

The outer add is better.

Add InstCombine tests. Remove one FIXME. Rename FIXME to TODO.

craig.topper retitled this revision from [InstCombine][LV][WIP] Fold (add (zext (add X, -1)), 1) -> (zext X) if X is non-zero. to [InstCombine][LV] Fold (add (zext (add X, -1)), 1) -> (zext X) if X is non-zero..Jan 30 2023, 1:54 PM
craig.topper added inline comments.
llvm/test/Transforms/InstCombine/add.ll
2877 ↗(On Diff #493409)

I will pre-commit these if they look ok. I wasn't sure if this was the right file or why we have add2.ll, add3.ll, add4.ll

spatel accepted this revision.Jan 30 2023, 2:33 PM

LGTM

llvm/test/Transforms/InstCombine/add.ll
2877 ↗(On Diff #493409)

I'm not sure what differentiates those test files either. We could use a lot of test dir cleanup in general, but it never seems to be high enough priority.

2901 ↗(On Diff #493409)

This will get canonicalized to add, or many other things will break. So there's not much value in this particular test IMO.

2927 ↗(On Diff #493409)

It seems unlikely to matter in practice, but I think the vector constant matching is inconsistent. We'll allow poison elements in the -1, but not the +1? Could add a test or two to show that gap.

This revision is now accepted and ready to land.Jan 30 2023, 2:33 PM
craig.topper added inline comments.Jan 30 2023, 3:42 PM
llvm/test/Transforms/InstCombine/add.ll
2901 ↗(On Diff #493409)

Ok I'll remove it. I took inspiration from the icmp_dec_assume_nonzero and icmp_dec_sub_assume_nonzero tests in icmp-add.ll which also use isKnownNonZero.

Remove sub test.
Add poison tests

craig.topper closed this revision.Jan 31 2023, 1:07 PM

Committed df76ff98e8e68ef00e61574ce6ac688e6e12b9df looks like I lost the differential revision comment when I commited.