This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by frasercrmck on Mar 30 2022, 9:32 AM.

Details

Summary

This patch adds the minimum required to successfully lower vp.icmp via
the new ISD::VP_SETCC node to RVV instructions.

Regular ISD::SETCC goes through a lot of canonicalization which targets
may rely on which has not hereto been ported to VP_SETCC. It also
supports expansion of individual condition codes and a non-boolean
return type. Support for all of that will follow in later patches.

In the case of RVV this largely isn't a problem as the vector integer
comparison instructions are plentiful enough that it can lower all
VP_SETCC nodes on legal integer vectors except for boolean vectors,
which regular SETCC folds away immediately into logical operations.

Floating-point VP_SETCC operations aren't as well supported in RVV and
the backend relies on condition code expansion, so support for those
operations will come in later patches.

Portions of this code were taken from the VP reference patches.

Diff Detail

Event Timeline

frasercrmck created this revision.Mar 30 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 9:32 AM
frasercrmck requested review of this revision.Mar 30 2022, 9:32 AM

Note I'm only testing i8, i32 and i64 with vscale x 1 and vscale x 8 because the test was getting silly. I thought it was better to test all the CCs and various LHS/RHS swappings than each vector type explicitly. If there are concerns about that then let me know!

Oh and I'll update the patch to test fixed-length vectors, which I'd forgotten.

  • add fixed-length tests
rogfer01 added inline comments.Apr 1 2022, 12:14 AM
llvm/include/llvm/IR/VPIntrinsics.def
312

Now that we're at it, could we replace this block with the following one?

BEGIN_REGISTER_VP_INTRINSIC(vp_icmp, 3, 4)
VP_PROPERTY_FUNCTIONAL_OPC(ICmp)
VP_PROPERTY_CMP(2, false)
END_REGISTER_VP_INTRINSIC(vp_icmp)

Should be equivalent and avoids defining an unnecessary ISD::VP_ICMP which we are not going to use.

(A similar thing can be done for VP_FCMP but we can do that in the patch with the lowering)

craig.topper added inline comments.Apr 1 2022, 2:31 PM
llvm/include/llvm/CodeGen/SelectionDAG.h
1086

This could be stricter. Everything needs to be a vector.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7521

Is the vp.fcmp intrinsic considered an FPMathOperator or not? I wouldn't expect it to be dynamic. I'm guess it's not because it doesn't return an FP type which is what the dyn_cast is checking

frasercrmck marked 2 inline comments as done.Apr 4 2022, 7:34 AM
frasercrmck added inline comments.
llvm/include/llvm/CodeGen/SelectionDAG.h
1086

Yep, good catch.

llvm/include/llvm/IR/VPIntrinsics.def
312

Good shout, thanks.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7521

I'd imagine that vp.fcmp should in theory be as much a FPMathOperator as fcmp is, but you're right about this never actually being a FPMathOperator due to the return type. I copied this particular code from the reference patch so I don't know if @simoll has other ideas. Looking again at the reference patch, I notice a change to classof to support VP intrinsics:

// Judge based on function.
auto *VPIntrin = dyn_cast<VPIntrinsic>(V);
if (VPIntrin) {
  auto OCOpt = VPIntrin->getFunctionalOpcode();
  Opcode = OCOpt ? *OCOpt : (unsigned) Instruction::Call;
}

Also fcmp being a FPMathOperator seems to be questionable:

// FIXME: To clean up and correct the semantics of fast-math-flags, FCmp
//        should not be treated as a math op, but the other opcodes should.
//        This would make things consistent with Select/PHI (FP value type
//        determines whether they are math ops and, therefore, capable of
//        having fast-math-flags).
case Instruction::FCmp:
  return true;

My initial reaction is to remove this bit for now and come back to it? This patch is only nominally supporting icmp anyway. What do you think?

frasercrmck marked 2 inline comments as done.

rebase & address feedback

craig.topper added inline comments.Apr 4 2022, 11:50 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7521

Let's remove it from this patch.

That FIXME on fcmp fast math flgs has been there for a while. I don't think we're close to fixing it yet.

  • drop impossible cast to FPMathOperator (with FIXME)
frasercrmck marked an inline comment as done.Apr 5 2022, 6:50 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7521

Alright, sounds good.

frasercrmck marked an inline comment as done.
  • rebase on top of already-committed setcc pats
This revision is now accepted and ready to land.Apr 6 2022, 9:00 AM
This revision was landed with ongoing or failed builds.Apr 6 2022, 9:03 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-setcc-int-vp.ll