This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeDAG] Implement promotion rules for SELECT_CC
ClosedPublic

Authored by LemonBoy on Mar 3 2021, 7:25 AM.

Details

Summary

Implement the promotion rule for SELECT_CC nodes by upcasting all the parameters and downcasting the result.
The AArch64 target makes use of this rule and, since it was not implemented, in some cases the instruction selector would hit an assertion upon encountering the illegal node.

This patch requires D97840, the included test cases hit both problems.

Diff Detail

Event Timeline

LemonBoy created this revision.Mar 3 2021, 7:25 AM
LemonBoy requested review of this revision.Mar 3 2021, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 7:25 AM
craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4729

I believe ISD::SELECT_CC can have integer operands for the compare and FP operands for the result and true/false value.

LemonBoy added inline comments.Mar 3 2021, 10:03 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4729

I actually tried to do so but hit an assertion when the previous node is RAUW'd.
The assert message is "Cannot replace uses of with self", some brief testing showed it complains after the SELECT_CC is promoted for the second time.

craig.topper added inline comments.Mar 3 2021, 10:41 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4729

Sorry, what exactly did you try to do? My point was only that you can't assume the same extend opcode is valid for all operands or that all operands want to be promoted to NVT.

LemonBoy added inline comments.Mar 3 2021, 10:57 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4729

Sorry, I misread your previous comment. I now get there are two possibly different types at play here.

Given the Promote rule kicks in according to the true/false type can we assume the comparison operands are already legalized? Should I try to expand those as well? And if so, is there a way to check if the comparison itself is legal?

craig.topper added inline comments.Mar 3 2021, 2:03 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4729

My best idea is to promote them if they happen to have the same type and leave them alone otherwise. I don't think there's a way to check that the comparison is legal.

LemonBoy updated this revision to Diff 328099.Mar 4 2021, 3:25 AM

Properly handle cases where the true/false operand have different types than the comparison ones.

craig.topper added inline comments.Mar 4 2021, 9:09 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4727

If NVT is Integer, but operand 0 and 1 are floating point, I'm not sure we should be calling isSignedIntSetCC. The floating point and integer condition codes are shared. So SETGT, SETLT, SETLE, SETGE are also valid for floating point, but they wouldn't mean anything for determining if you should SIGN_EXTEND or ZERO_EXTEND the true/false values of the SELECT_CC.

But I guess since this hasn't worked properly before now maybe this doesn't happen and it doesn't matter.

LemonBoy updated this revision to Diff 328223.Mar 4 2021, 10:09 AM

Assert all the operand types are the same.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4727

I see, this is trickier than anticipated. I'll simply assert that both types are equal and carry on, this promotion rule is rarely used and the only instances I could produce fall into this category.

This revision is now accepted and ready to land.Mar 4 2021, 10:43 AM
This revision was landed with ongoing or failed builds.Mar 5 2021, 9:23 AM
This revision was automatically updated to reflect the committed changes.