This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] don't widen an arbitrary sequence of vector ops (PR40032)
ClosedPublic

Authored by spatel on Dec 16 2018, 9:07 AM.

Details

Summary

The problem is shown specifically for a case with vector multiply here:
https://bugs.llvm.org/show_bug.cgi?id=40032
...and this might mask the original backend bug for ARM shown in:
https://bugs.llvm.org/show_bug.cgi?id=39967

As the test diffs here show, we were (and probably still aren't) doing these kinds of transforms in a principled way. We are producing more or equal wide instructions than we started with in some cases, so we still need to restrict/correct other transforms from overstepping.

If there are perf regressions from this change, we can either carve out exceptions to the general IR rules, or improve the backend to do these transforms when we know the transform is profitable. That's probably similar to a change I just made in D55448.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Dec 16 2018, 9:07 AM
RKSimon added inline comments.Dec 16 2018, 10:08 AM
lib/Transforms/InstCombine/InstCombineCasts.cpp
1117 ↗(On Diff #178401)

What about putting !DestTy->isVectorTy() inside InstCombiner::shouldChangeType ?

spatel marked 2 inline comments as done.Dec 17 2018, 4:19 AM
spatel added inline comments.
lib/Transforms/InstCombine/InstCombineCasts.cpp
1117 ↗(On Diff #178401)

Yes, that's cleaner - currently we just assert that we have scalar inside there.

spatel updated this revision to Diff 178441.Dec 17 2018, 4:21 AM
spatel marked an inline comment as done.

Patch updated:
Move vector type check into shouldChangeType() rather than asserting we have scalars.

No more comments from me

This revision is now accepted and ready to land.Dec 17 2018, 11:26 AM
This revision was automatically updated to reflect the committed changes.