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

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

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

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

2927

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

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.