Page MenuHomePhabricator

[LegalizeDAG][RISCV][PowerPC][AMDGPU][WebAssembly] Improve expansion of SETONE/SETUEQ on targets without SETO/SETUO.
ClosedPublic

Authored by craig.topper on Jan 11 2021, 1:55 PM.

Details

Summary

If SETO/SETUO aren't legal, they'll be expanded and we'll end up
with 3 comparisons.

SETONE is equivalent to (SETOGT || SETOLT)
so if one of those operations is supported use that expansion. We
don't need both since we can commute the operands to make the other.

SETUEQ can be implemented with !(SETOGT || SETOLT) or (SETULE && SETUGE).
I've only implemented the first because it didn't look like most of the
affected targets had legal SETULE/SETUGE.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 11 2021, 1:55 PM
craig.topper requested review of this revision.Jan 11 2021, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 1:55 PM
tlively accepted this revision.Jan 11 2021, 2:53 PM

Nice! this looks like a good improvement for WebAssembly at least.

This revision is now accepted and ready to land.Jan 11 2021, 2:53 PM
frasercrmck accepted this revision.Jan 12 2021, 2:37 AM

LGTM other than my suggestion.

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

Minor, but maybe we could put into the comments the part of the commit message explaining why only one of OGT/OLT have to be legal for this to be worthwhile? I think that's quite useful background for anyone coming across this in the future.

nemanjai accepted this revision.Jan 12 2021, 4:54 AM

LGTM for PPC. Thanks for this improvement.

Looking good on the RISC-V side.

Would you mind removing the out-of-date TODOs? They were out of date before, but with this change they're even more out of date.

llvm/test/CodeGen/RISCV/double-br-fcmp.ll
307–308

Please can you remove this TODO, as it is no longer relevant?

llvm/test/CodeGen/RISCV/double-select-fcmp.ll
208–209

And here.

llvm/test/CodeGen/RISCV/float-br-fcmp.ll
284–285

and here

llvm/test/CodeGen/RISCV/float-select-fcmp.ll
167–168

And this one

llvm/test/CodeGen/RISCV/half-br-fcmp.ll
260–261

And here!

llvm/test/CodeGen/RISCV/half-select-fcmp.ll
137–138

and here!

This revision was landed with ongoing or failed builds.Jan 12 2021, 10:52 AM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Jan 14 2021, 11:03 AM
bjope added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3964

NeedInvert is not handled here so this assert started to fail for our OOT target.

Haven't really investigate it yet. But should the code here support NeedInvert (or one should tell LegalizeSetCCCondCode above that legalization using NeedInvert isn't allowed).

craig.topper added inline comments.Jan 14 2021, 11:21 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3964

I think it's not possible to invert it if Tmp4.getNode() is true below. But I think it should be false for the case this patch introduced.

So I think we can move the assert into the if at line 3969. And pick ISD::SETEQ instead of ISD::SETNE on line 3973 if NeedInvert is true.

bjope added inline comments.Jan 14 2021, 12:10 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3964

Right. Thanks!
I'll play around with that tomorrow.

bjope added inline comments.Jan 15 2021, 4:00 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3964