This is an archive of the discontinued LLVM Phabricator instance.

[DAG] expandIntMINMAX - attempt to match existing SETCC node
ClosedPublic

Authored by RKSimon on Mar 1 2023, 6:12 AM.

Details

Summary

As noticed on D144789, when we have pairs of min/max nodes we often end up with multiple comparisons which we could reuse with commuted select ops, so check to see if a suitable SETCC already exists. This also allowed us to remove a similar X86 peephole.

There are other getSETCC cases where we could safely use other CondCodes as well - I've been trying to think of how we could reuse this logic from SelectionDAG but haven't found anything that always works well.

An alternative would be to have a TLI callback that returns a preferred CondCode from a list of options, I've noticed this helped fpclamptosat tests on some other targets (MVE + WebAssembly), but other tests suffered.

Diff Detail

Event Timeline

RKSimon created this revision.Mar 1 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 6:12 AM
RKSimon requested review of this revision.Mar 1 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 6:12 AM
Herald added a subscriber: aheejin. · View Herald Transcript
RKSimon added inline comments.Mar 1 2023, 6:19 AM
llvm/test/CodeGen/X86/vselect-minmax.ll
10326

SSE BLENDV's xmm0 constraints :(

spatel accepted this revision.Mar 1 2023, 8:42 AM

LGTM

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9568

Make the fail string more informative:

llvm_unreachable("Expected max/min opcode");

...could also make that the default case of the switch.

This revision is now accepted and ready to land.Mar 1 2023, 8:42 AM
goldstein.w.n added inline comments.Mar 1 2023, 8:49 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9542

Maybe there should be a TLI flag to do PrefCommuteCC before AltCC?

RKSimon added inline comments.Mar 1 2023, 9:04 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9542

I think that's where we're heading towards - x86 might be the most limited of ISAs for icmp (vectors anyway), and SETCC reuse seems to be an important part of this, but always choosing the target's simplest CondCode is vital as well.