This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][AArch64][SVE] Perform SETCC condition legalization in LegalizeVectorOps
ClosedPublic

Authored by bsmith on Mar 19 2021, 4:31 AM.

Details

Summary

This is currently performed in SelectionDAGLegalize, here we make it also
happen in LegalizeVectorOps, allowing a target to lower the SETCC condition
codes first in LegalizeVectorOps and then lower to a custom node afterwards,
without having to duplicate all of the SETCC condition legalization in the
target specific lowering.

As a result of this, fixed length floating point SETCC nodes can now be
properly lowered for SVE.

Diff Detail

Event Timeline

bsmith created this revision.Mar 19 2021, 4:31 AM
bsmith requested review of this revision.Mar 19 2021, 4:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 4:31 AM
craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1384

Can we call DAG.getBoolConstant here?

bsmith updated this revision to Diff 332342.Mar 22 2021, 10:02 AM
  • Fixed failures in PPC/RISCV tests due to slightly different codegen.
  • Use getBoolConstant instead of manually constructing true/false values.
bsmith marked an inline comment as done.Mar 22 2021, 10:02 AM
david-arm added inline comments.Mar 24 2021, 6:40 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
773

Do we need to do something similar for SELECT_CC here too?

1346

Probably makes sense to declare these in the same place they are assigned?

1350

Is it worth doing an early return here for the inverse instead? For example,

if (TLI.getCondCodeAction(CCCode, OpVT) != TargetLowering::Expand) {
  Results.push_back(UnrollVSETCC(Node));
  return;
}
...

That way you can avoid the extra indentation.

1352

Perhaps worth renaming these to LHS, RHS and CC so it's more readable?

1361

If it's legal and doesn't need inverting do we even need to reassign Tmp1?

llvm/test/CodeGen/AArch64/sve-fixed-length-float-compares.ll
347

I'm not sure where the i8 comes into the name here - should this be fcmp_ueq_v16f16 instead? Same comment for other functions below.

bsmith updated this revision to Diff 333270.Mar 25 2021, 5:40 AM
bsmith marked 5 inline comments as done.
  • Remove unfinished case for SELECT_CC, this isn't needed currently for SVE, can be added later if/when needed.
  • Tidy up code in VectorLegalizer::ExpandSETCC.
  • Fix bogus test names.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1361

This is for when the operands are swapped, LegalizeSetCCCondCode will modify the LHS/RHS input parameters in this case, hence the reassignment is to allow for that.

david-arm accepted this revision.Mar 26 2021, 9:20 AM

LGTM! Thanks for the changes!

This revision is now accepted and ready to land.Mar 26 2021, 9:20 AM
paulwalker-arm accepted this revision.Mar 29 2021, 7:22 AM
This revision was landed with ongoing or failed builds.Mar 29 2021, 7:42 AM
This revision was automatically updated to reflect the committed changes.