This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] implement GPR (U/S)(MIN/MAX) instr support
ClosedPublic

Authored by stuij on Dec 6 2022, 5:33 AM.

Details

Summary

Lower umin, umax, smin, smax intrinsics to corresponding UMIN, UMAX, SMIN, SMAX
instructions when feat CSSC is available.

Diff Detail

Event Timeline

stuij created this revision.Dec 6 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 5:33 AM
stuij requested review of this revision.Dec 6 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 5:33 AM
stuij added a comment.Dec 6 2022, 6:53 AM

I have tried to collapse the to check test output for the different run configuration as we discussed on the SelDag ticket for this (D138813). However I could not wrangle the prefixes in a configuration that would make the update script output sensible checks that would cover all 4 cases. Hand-crafting is not a good route I think, especially since even though spelling out all 4 outputs is a bit verbose, it doesn't make things less clear, which should be the more important measure I think.

paquette added inline comments.Dec 7 2022, 2:08 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
954 ↗(On Diff #480446)

Can you add a test for this function in llvm/unittests/CodeGen/GlobalISel/LegalizerInfoTest.cpp?

Then maybe split this into two patches:

Patch 1: Add minScalarIf
Patch 2: Add the new lowering

stuij updated this revision to Diff 483953.Dec 19 2022, 6:54 AM

spun out minScalarIf definition into it's own patch

stuij retitled this revision from [AArch64] implement GPR (U/S)(MIN/MAX) instruction GISel support to [GlobalISel][Legalizer] add minScalarIf action.Dec 19 2022, 6:55 AM
stuij edited the summary of this revision. (Show Details)
stuij marked an inline comment as done.
stuij added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
954 ↗(On Diff #480446)

thanks, I made a separate patch for minScalarIf.

stuij retitled this revision from [GlobalISel][Legalizer] add minScalarIf action to [AArch64][GlobalISel] implement GPR (U/S)(MIN/MAX) instr support.Dec 19 2022, 6:58 AM
arsenm added a subscriber: arsenm.Dec 19 2022, 7:12 AM
arsenm added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
196–201

Should prefer to check all legal cases first

stuij updated this revision to Diff 483965.Dec 19 2022, 7:40 AM
stuij marked an inline comment as done.

flip around .legalIf and .minScalarIf in the action definition builder

stuij marked an inline comment as done.Dec 19 2022, 7:41 AM
arsenm added inline comments.Dec 19 2022, 9:15 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
193–201

You can also conditionally add the predicates based on the subtarget feature check. i.e. you don't need to check the predicates inside the lambda, you can append rules for different subtargets.

stuij marked an inline comment as done.Dec 21 2022, 2:53 AM
stuij added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
193–201

Right. Indeed, I didn't think of that. Reviewing I still prefer the lambda-inlining. Maybe because I'm giddy to finally have an excuse to write lots of lambdas in C++. From your phrasing I think you're also ok with the current setup.

stuij marked an inline comment as done.Dec 21 2022, 2:56 AM
arsenm added inline comments.Dec 21 2022, 4:43 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
193–201

These are called a lot

stuij updated this revision to Diff 484835.Dec 22 2022, 7:28 AM

unlambdify actions

stuij marked an inline comment as done.Dec 22 2022, 7:30 AM
stuij added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
193–201

If you're happy with this, I'll do the same to the other outstanding patches, lambda's be damned.

aemerson accepted this revision.Jan 5 2023, 12:32 AM
aemerson added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
193–201

I think Matt's right in general, although IMO if it becomes extremely unwieldy for some reason we can embed the checks into the predicates (its ultimately not a big deal with these uncommon operations).

This revision is now accepted and ready to land.Jan 5 2023, 12:32 AM
This revision was landed with ongoing or failed builds.Jan 5 2023, 3:50 AM
This revision was automatically updated to reflect the committed changes.
stuij marked an inline comment as done.