This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add intrinsics for vmv.x.s and vmv.s.x
ClosedPublic

Authored by craig.topper on Dec 15 2020, 9:13 PM.

Details

Summary

This adds intrinsics for vmv.x.s and vmv.s.x.

I've used stricter type constraints on these intrinsics than what we've been doing on the arithmetic intrinsics so far. This will allow us to not need to pass the scalar type to the Intrinsic::getDeclaration call when creating these intrinsics.

A custom ISD is used for vmv.x.s in order to implement the change in computeNumSignBitsForTargetNode which can remove sign extends on the result.

I also modified the MC layer description of these instructions to show the tied source/dest operand. This is different than what we do for masked instructions where we drop the tied source operand when converting to MC. But it is a more accurate description of the instruction. We can't do this for masked instructions since we use the same MC instruction for masked and unmasked. Tools like llvm-mca operate in the MC layer and rely on ins/outs and Uses/Defs for analysis so I don't know if we'll be able to maintain the current behavior for masked instructions. So I went with the accurate description here since it was easy.

Diff Detail

Event Timeline

craig.topper created this revision.Dec 15 2020, 9:13 PM
craig.topper requested review of this revision.Dec 15 2020, 9:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 9:13 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Dec 15 2020, 9:15 PM
llvm/test/CodeGen/RISCV/rvv/vmv.x.s-rv32.ll
7

The signext attribute on the results here gives test coverage on the computeNumSignBitsForTargetNode change.

Rebase and use anyint_ty instead of anyvector_ty to be consistent with an upcoming vfmv.s.f/vfmv.f.s patch

HsiangKai added inline comments.Dec 17 2020, 12:26 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1393

Do we need to add a constraint "$rd = $rs1"?

craig.topper added inline comments.Dec 17 2020, 12:52 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1393

Yes we do. I thought I had done that. Thanks!

Add tied register constraint. Remove unnecessary type from intrinsic names in tests.

frasercrmck accepted this revision.Dec 17 2020, 1:32 AM

LGTM. Was it a TableGen type contradiction error that you saw without the custom node?

This revision is now accepted and ready to land.Dec 17 2020, 1:32 AM

LGTM. Was it a TableGen type contradiction error that you saw without the custom node?

Well I thought it was, but after debugging a different issue with fmv it might not have been. The DAG isel patterns don't know about LLVMVectorElementType<0> so the change to anyint_ty might have fixed it.

Well I thought it was, but after debugging a different issue with fmv it might not have been. The DAG isel patterns don't know about LLVMVectorElementType<0> so the change to anyint_ty might have fixed it.

Oh neat, is it worth trying without the custom nodes, then?

Remove custom node for vmv.s.x. Using llvm_anyint_ty for the vector instead of llvm_anyvector_ty made the isel table work after all.

craig.topper requested review of this revision.Dec 17 2020, 1:48 AM
craig.topper edited the summary of this revision. (Show Details)

Update comment I missed when removing one of the custom ISD opcodes

@frasercrmck does this look ok to you with the custom node removed?

frasercrmck accepted this revision.Dec 18 2020, 1:50 AM

Yep, LGTM.

This revision is now accepted and ready to land.Dec 18 2020, 1:50 AM
This revision was landed with ongoing or failed builds.Dec 18 2020, 10:31 AM
This revision was automatically updated to reflect the committed changes.