This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering] Don't use ISD::SELECT_CC in expandFP_TO_INT_SAT.
ClosedPublic

Authored by craig.topper on Apr 28 2023, 1:32 PM.

Details

Summary

This function gets called for vectors and ISD::SELECT_CC was never
intended to support vectors. Some updates were made to support
it when this function started getting used for vectors.

Overall, using separate ISD::SETCC and ISD::SELECT looks like an
improvement even for scalar.

Diff Detail

Event Timeline

craig.topper created this revision.Apr 28 2023, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 1:32 PM
craig.topper requested review of this revision.Apr 28 2023, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 1:32 PM
craig.topper added inline comments.Apr 28 2023, 3:59 PM
llvm/test/CodeGen/RISCV/float-convert.ll
537

The xori+addi here are equivalent to neg. I'll fix this in a follow up patch.

Rebase after fixing (addi (xor (setcc X, Y), 1) -1) on RISC-V

RKSimon accepted this revision.Apr 29 2023, 9:57 AM

LGTM

This revision is now accepted and ready to land.Apr 29 2023, 9:57 AM
nikic added a subscriber: nikic.Apr 29 2023, 10:05 AM

Why is SELECT_CC a thing at all?

Why is SELECT_CC a thing at all?

On targets where compare instructions modify a flag register, "SELECT" and "BRCOND" aren't really the natural representation; it effectively makes x != 0 a special case. SELECT_CC and BR_CC represent the combination of a comparison followed by a branch.

That said, a lot of of targets with a flag register prefer to to custom-lower SETCC anyway, so maybe it's not as useful today.

Why is SELECT_CC a thing at all?

On targets where compare instructions modify a flag register, "SELECT" and "BRCOND" aren't really the natural representation; it effectively makes x != 0 a special case. SELECT_CC and BR_CC represent the combination of a comparison followed by a branch.

FWIW on such targets neither of these are natural. CC versions are just slightly
easier to lower (saves a couple lines of code).
I wish these nodes were not there when I started bringing up a new backend
and that I didn't read the comments saying "CC" versions are more natural.
This would save me quite a lot of time.

That said, a lot of of targets with a flag register prefer to to custom-lower SETCC anyway, so maybe it's not as useful today.

These nodes are just a redundancy (like a "not x" compared to "xor x, ~0").
I'm glad RISC-V doesn't use them, even though BR_CC is pertty much natural.