This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Allow the ArtifactValueFinder to return the best available register on failure.
ClosedPublic

Authored by aemerson on Jul 28 2021, 5:34 PM.

Details

Summary

In some cases, like with inserts, we may have a matching size register already, but still decide to try to look further. This change adds a CurrentBest register to the value finder state, and any time a method fails to make progress, returns that register (which may just be an empty Register).

To facilitate this, add a new entry point to the findValueFromDef() function which initializes this state.

Also fix the build vector finder to return the current build_vector if all sources are being requested.

Diff Detail

Event Timeline

aemerson created this revision.Jul 28 2021, 5:34 PM
aemerson requested review of this revision.Jul 28 2021, 5:34 PM

I'm not convinced this is the best approach actually. Might be better to keep track of the best register as a private member, and then have a single external API entry point for queries that resets the fail reg each time.

aemerson updated this revision to Diff 362638.Jul 28 2021, 10:16 PM
aemerson edited the summary of this revision. (Show Details)

New version does the above.

foad added inline comments.Aug 2 2021, 12:51 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
757–760

Surely DefReg is always a safe "best" return value? Why would you ever need to return Register() instead?

aemerson added inline comments.Aug 2 2021, 1:39 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
757–760

I suppose it's a matter of API design/opinion. If you get a valid Register back, then you know it's going to be different from the existing register so you can RAUW it. If it can return DefReg, then you need to check if it's different first, otherwise you'll trigger lots of unnecessary observer invalidations.

foad added inline comments.Aug 3 2021, 1:14 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
757–760

If you get a valid Register back, then you know it's going to be different from the existing register

I'm confused: the comment specifically says that you might get DefReg back, which is what you passed in.

aemerson added inline comments.Aug 3 2021, 5:04 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
757–760

I think that's an obsolete comment, and I actually need to change the users to reflect the semantics.

aemerson updated this revision to Diff 363976.Aug 3 2021, 11:50 PM

Change API to return an empty Register if the best register found is the same as the one being examined.

arsenm added inline comments.Aug 5 2021, 4:45 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
586–588

Comment is out of date?

arsenm accepted this revision.Aug 5 2021, 4:49 PM
This revision is now accepted and ready to land.Aug 5 2021, 4:49 PM
This revision was landed with ongoing or failed builds.Aug 5 2021, 5:37 PM
This revision was automatically updated to reflect the committed changes.