This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][VP] Add basic RVV codegen for vp.fcmp
ClosedPublic

Authored by frasercrmck on Apr 4 2022, 10:34 AM.

Details

Summary

This patch adds the necessary infrastructure to lower vp.fcmp via
ISD::VP_SETCC to RVV instructions.

Most notably this patch adds cond-code legalization for VP_SETCC,
reusing the existing TargetLowering::LegalizeSetCCCondCode by passing in
additional SDValue parameters for the Mask and EVL. This method then
uses VP operations to legalize the condcode.

There is still a general lack of canonicalization on VP_SETCC as opposed
to SETCC which results in worse code than is theoretically possible.

Diff Detail

Event Timeline

frasercrmck created this revision.Apr 4 2022, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 10:34 AM
frasercrmck requested review of this revision.Apr 4 2022, 10:34 AM

remove VP_FCMP as per VP_ICMP
ensure VP_SETCC after legalizing, preserving EVL in more cases

  • add a VP logical-not helper, preserving mask/evl better

There is still a general lack of canonicalization on VP_SETCC as opposed
to SETCC which results in worse code than is theoretically possible.

What canonicalizations are missing. For integer, I know we're missing canonicalizing constants to RHS, but we don't do that for FP ISD::SETCC.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3654–3655

Not related to this patch, but is this equivalent to SelectionDAG::getBoolConstant with true for the V parameter? If we agree it is, I'll write a patch to replace it.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9210

It would be an error if only one was SDValue() right? Should we assert that?

add assert that mask/evl are both set or unset

frasercrmck marked an inline comment as done.Apr 5 2022, 8:32 AM

What canonicalizations are missing. For integer, I know we're missing canonicalizing constants to RHS, but we don't do that for FP ISD::SETCC.

I was thinking of some things in TargetLowering::SimplifySetCC which does a couple of FP-specific things on constants and SelectionDAG::FoldSetCC which does too. But you're right these are far fewer than for integers, and maybe they're technically not canonicalizations?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3654–3655

Makes sense to me, yeah.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9210

Ah yeah so it would. Not sure what I was thinking.

frasercrmck marked an inline comment as done.

rebase

LGTM other than the assert and the comments about the IsVP computation.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9211

I don't think this is assert is complete. It's only checking that Mask is null when EVL is null. If EVL is non-null it doesn't check Mask.

Something like assert(!EVL == !Mask) would check both conditions.

9212

I'm debating whether we need the "!= SDValue()" SDValue has an operator bool. But bool isVP = EVL might not be very readable. There's also bool isVP = !!EVL but I don't know if that's any better.

I only found 1 file in tree ARMISelLowering.cpp that does == SDValue() or != SDValue()

craig.topper accepted this revision.Apr 6 2022, 4:58 PM
This revision is now accepted and ready to land.Apr 6 2022, 4:58 PM
frasercrmck marked 2 inline comments as done.Apr 7 2022, 12:27 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
9211

Right you are, thanks!

9212

Yeah I'd considered all of those but wasn't happy with any one of them. It's a good point that ==/!= SDValue() isn't common enough to warrant its usage though. I suppose bool IsNonVP = !EVL is both the more common situation and a (arguably) more common use of the operator bool, but then again even = !X is rare in the codebase.

I'll go with IsNonVP as it also matches the form now used in the assert.

This revision was landed with ongoing or failed builds.Apr 7 2022, 1:27 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked 2 inline comments as done.