This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Combine select + fcmp to fminnum/fmaxnum/fminimum/fmaximum
ClosedPublic

Authored by paquette on Jan 5 2022, 2:05 PM.

Details

Summary

This is a partial port of the code used by the SelectionDAGBuilder to translate selects.

In particular, see matchSelectPattern in ValueTracking.cpp. This is a GISel-equivalent of the portion which handles fminnum/fmaxnum/fminimum/fmaximum.

I tried to set it up so it'd be easy to add the non-FP cases. Those are simpler. On the AArch64-end, it seems like the FP cases are more important for perf right now, so I bit the bullet and went at the more complicated problem. :)

I elected to do this as a post-legalize combine rather than in the IRTranslator because

  • Deciding which fmax/fmin to use can depend on legalization rules
  • Philosophically-speaking (TM), putting it in a combine just feels cleaner
  • Being able to enable/disable the combine is handy

Another option would be to use the ValueTracking code in the IRTranslator and match what SelectionDAGBuilder::visitSelect does. I think that may be somewhat annoying since we'd need to write lowerings back into the selects in the legalizer. I'm not strongly opposed to the approach.

We'd also want to be careful with vector selects once that's implemented, which explicitly check if a vector select is legal on the target. That'd probably need a hook.

From what I can tell, doing this as a combine is probably a cleaner option long-term.

Diff Detail

Event Timeline

paquette created this revision.Jan 5 2022, 2:05 PM
paquette requested review of this revision.Jan 5 2022, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 2:05 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Jan 5 2022, 2:15 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5648–5649

The 0 behavior is specified as ordering -0<0 for FMAXIMUM/FMINIMUM

5682

Leftover debug printing

paquette updated this revision to Diff 398012.Jan 6 2022, 4:24 PM
  • Allow 0 for G_FMAXIMUM/G_FMINIMUM cases
  • Remove debug code
tschuett added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
796

I prefer enum class for new code, but it is up to you.

aemerson added inline comments.Jan 10 2022, 4:44 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5573

Can this be the actual Predicate type instead of unsigned?

5601–5604

What's the reason for choosing this particular ordering of legality checks such that we prefer G_FMINNUM?

paquette added inline comments.Jan 10 2022, 4:49 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5601–5604

I was just matching the behaviour in SelectionDAGBuilder so I'm not entirely sure:

case SPNB_RETURNS_ANY: {
  if (TLI.isOperationLegalOrCustom(ISD::FMINNUM, VT))
    Opc = ISD::FMINNUM;
  else if (TLI.isOperationLegalOrCustom(ISD::FMINIMUM, VT))
    Opc = ISD::FMINIMUM;

Maybe @arsenm knows better since I think he wrote the original code. (https://reviews.llvm.org/rGfabab4b7dd7d4ccefec2bb6cd405044429637ba6)

arsenm added inline comments.Jan 18 2022, 8:59 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5601–5604

FMINIMUM/FMAXIMUM are not widely used/available. The DAG version is more strict in checking for isLegalOrCustom, whereas here you're forming them before legalize anyway.

I think this should follow that and look for some positive legality only. The trickier case I'm worried about is on vector types which will be split later. We probably should have a utility that looks like the getTypeAction loop for finding the ultimate legalized type

arsenm added a comment.Apr 5 2022, 7:12 PM

Reverse ping

Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 7:12 PM

Second reverse ping.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5601–5604

Since this is handling RETURNS_ANY before legalization seems fine to me?

paquette updated this revision to Diff 447504.Jul 25 2022, 3:59 PM

rebase + check isLegal + use enum class

aemerson accepted this revision.Jul 25 2022, 11:18 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5792

You can just cast to GSelect and use the convenience methods.

This revision is now accepted and ready to land.Jul 25 2022, 11:18 PM
foad added a comment.Jul 26 2022, 1:09 AM

Just curious: why do we need to do this in isel? Is there a reason it cannot or does not get done at the IR level?

Just curious: why do we need to do this in isel? Is there a reason it cannot or does not get done at the IR level?

I think this is one where we need both. First, selects are likely to appear out of legalization. Second, the IR doesn't have the full range of min/max opcodes (it doesn't have the _IEEE versions, and the intrinsics were incorrectly named)

This revision was landed with ongoing or failed builds.Sep 16 2022, 1:36 PM
This revision was automatically updated to reflect the committed changes.