Page MenuHomePhabricator

[RISCV] Optimize select_cc after fp compare expansion
ClosedPublic

Authored by craig.topper on Jan 12 2021, 12:44 PM.

Details

Summary

Some FP compares expand to a sequence ending with (xor X, 1) to invert the result. If
the consumer is a select_cc we can likely get rid of this xor by fixing
up the select_cc condition.

This patch combines (select_cc (xor X, 1), 0, setne, trueV, falseV) -
(select_cc X, 0, seteq, trueV, falseV) if we can prove X is 0/1.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 12 2021, 12:44 PM
craig.topper requested review of this revision.Jan 12 2021, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 12:44 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper edited the summary of this revision. (Show Details)Jan 12 2021, 12:45 PM
craig.topper edited the summary of this revision. (Show Details)

For a moment I thought this was doing what D94535 is, but more elegantly, but I now see it's for SELECT, not brcond.

jrtc27 added inline comments.Jan 12 2021, 12:59 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1833–1834

Is there a reason not to generalise this to be any condition code and its negation?

1840

Hm, do we need it to have just one use? I could imagine it being better for instruction scheduling to be able to do:

%0 = ...
%1 = xor %0, 1
%2 = select_cc %0, 0, seteq, trueV, falseV
%3 = ... %1 ...

even though it's not a code size win, though it does come at the expense of slightly increased register pressure.

craig.topper added inline comments.Jan 12 2021, 1:07 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1833–1834

Not all condition codes are supported on select_cc. See normaliseSetCC. I could handle ISD::SETEQ->ISD::SETNE as well. I'd have to think through the logic for other comparisons.

1840

True. It's also possible to have more than 1 select_cc with the same condition.

-Remove one use check.
-Handle SETEQ as well, but I don't know how to test it as it requires an xor to appear after type legalization.

lenary added inline comments.Jan 14 2021, 9:38 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1831

We should absolutely document that this expects the condition to be an xleni with values corresponding to getBooleanContents. This should go where the opcode is defined in RISCVISelLowering.h though.

Given this seems to be the case, I think this optimization is correct (or as correct as my patch).

craig.topper added inline comments.Jan 14 2021, 10:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1831

The operands of SELECT_CC aren't required to follow getBooleanContents. There is no condition input. The inputs are the left and right hand side of a comparison. Those can have any value. We use MaskedValueIsZero to check if the input to an xor on the LHS has only 2 possible values(0/1). Knowing that tells us the xor is just flipping those 2 values.

For your brcond patch, I think it is true that the condition matches getBooleanContents.

Add documentation to SELECT_CC opcode

lenary accepted this revision.Jan 14 2021, 1:23 PM

LGTM! Thanks!

This revision is now accepted and ready to land.Jan 14 2021, 1:23 PM
This revision was landed with ongoing or failed builds.Jan 14 2021, 1:48 PM
This revision was automatically updated to reflect the committed changes.