This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Define vadd/vsub/vrsub intrinsics and lower to V instructions.
ClosedPublic

Authored by HsiangKai on Dec 10 2020, 1:54 AM.

Details

Summary

Demonstrate how to model RISC-V V intrinsics and lower them to V instructions.

The CodeGen strategy is designed by @rogfer01 from BSC.

Diff Detail

Event Timeline

HsiangKai created this revision.Dec 10 2020, 1:54 AM
HsiangKai requested review of this revision.Dec 10 2020, 1:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 1:54 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
frasercrmck added inline comments.Dec 10 2020, 2:22 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
17

Documenting this and why it's necessary (X0 != ADD X0, 0 for vsetvli) would be good

78

Documenting this helper class would be helpful

llvm/test/CodeGen/RISCV/rvv/vadd.ll
3

Having +m,+f,+d,+a,+c all seem unnecessary here

7

The formatting of this IR function declaration looks a little weird to me - is this the usual style?

19

It would be good to have a test without undef inputs.

HsiangKai updated this revision to Diff 310849.Dec 10 2020, 4:55 AM

Add more test cases and extend scalar operand to the legal type.

HsiangKai updated this revision to Diff 310860.Dec 10 2020, 5:28 AM

Address @frasercrmck's comments.

HsiangKai added inline comments.Dec 10 2020, 5:29 AM
llvm/test/CodeGen/RISCV/rvv/vadd.ll
7

It is generated by our internal test generator.

kito-cheng added inline comments.Dec 10 2020, 5:42 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2305

I would suggest add a new assert here, the assertion message seems not fit for the extra checking.

HsiangKai updated this revision to Diff 310866.Dec 10 2020, 5:52 AM
  • Remove unnecessary swap function in this patch. Add it if we need it afterward.
  • Address @kito-cheng's comments.
rogfer01 added inline comments.Dec 10 2020, 8:49 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2067

Thanks a lot for the patch @HsiangKai !

I would add a comment here, to avoid confusion, stating that this is an interim calling convention and it may be changed in the future.

HsiangKai edited the summary of this revision. (Show Details)Dec 10 2020, 9:13 AM
HsiangKai updated this revision to Diff 310933.Dec 10 2020, 9:16 AM

Address @rogfer01's comments.

craig.topper added inline comments.Dec 10 2020, 9:23 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
26

The comment block above this is about atomics which should stay with the atomic intrinsics. Can we start a new section for vectors after closing the "let TargetPrefix = "riscv" in {" portion for atomics. Same as what I did in D92973.

73

The vl parameter is going to need to be llvm_anyint_ty to support RV32

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2121–2122

Can you pass TLI by reference since its never null? Just need to change the callers to pass *this instead of this

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
70

Why do we need to do this? The vector instructions for that use a FPR64 or FPR32 should only be supported when the scalar type is supported. Why can't we use the proper register class directly?

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.cpp
17 ↗(On Diff #310866)

This feels like a library layering violation. This file belongs to the MC layer and shouldn't be pulling in files in the IR directory.

llvm/test/CodeGen/RISCV/rvv/vadd.ll
19

Agreed. Can we just avoid undef inputs in all tests?

craig.topper added inline comments.Dec 10 2020, 9:36 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2257

Will RC ever be a subclass? Can we just check RC == &RISCV::VRRegClass?

2427

Would getRegClassFor work for the scalar case too?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
24

Put this inside the if since its only needed if we create nodes.

26

This can be simplified slightly to

auto *C = dyn_cast<ConstantSDNode(N);
if (C && C->isNullValue()

Then you don't need two levels of indentation

jrtc27 added inline comments.Dec 10 2020, 10:03 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
26

I forget whether C++17 is allowed; if so you can do

if (auto *C = dyn_cast<ConstantSDNode>(N); C->isNullValue())

to get the benefits both of reduced scope and of reduced indentation.

craig.topper added inline comments.Dec 10 2020, 10:05 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
70

Answering my own question. SEW is part of the pseudo operands. So we don't have separate pseudos for FPR16/FPR32/FPR64. That's why we have this

I'm not completely sure about extracting FPR64 values to FPR32 when we really care about the full 64-bit value. Inserting FPR16 to FPR32 seems more ok. How many instructions does this affect?

craig.topper added inline comments.Dec 10 2020, 1:27 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1017

Use SmallVector with the expected number of operands.

1022

MVT::i64 here should be Subtarget->XLenVT() I think.

craig.topper added inline comments.Dec 10 2020, 1:42 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
26

I think we're only allowed to use C++14.

HsiangKai updated this revision to Diff 311076.Dec 10 2020, 5:30 PM

Combine D93048 into this patch.

HsiangKai retitled this revision from [RISCV] Define vadd intrinsics and lower to V instructions. to [RISCV] Define vadd/vsub/vrsub intrinsics and lower to V instructions..Dec 10 2020, 5:34 PM
evandro added inline comments.Dec 10 2020, 6:06 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
20
s/~0/VLMAX/
21

"Keep current vl, just change vtype"

28

Subtarget->XLenVT() instead of MVT::i64, here and below, yes?

HsiangKai updated this revision to Diff 311093.Dec 10 2020, 7:09 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
70

@rogfer01, could you help to give a more detail explanation here, thanks.

HsiangKai updated this revision to Diff 311097.Dec 10 2020, 7:36 PM
HsiangKai updated this revision to Diff 311101.Dec 10 2020, 8:04 PM

The destination vector register group for a masked vector instruction cannot overlap the source mask register (v0), unless the destination vector register is being written with a mask value (e.g., comparisons) or the scalar result of a reduction.

Rebase on master.

Update test cases.

HsiangKai marked 17 inline comments as done.Dec 10 2020, 11:44 PM

Gather the class attributes.

Encode the destination type into pattern classes.

HsiangKai updated this revision to Diff 311136.Dec 11 2020, 1:15 AM

Refine the class name in IntrinsicsRISCV.td.

HsiangKai updated this revision to Diff 311175.Dec 11 2020, 5:11 AM
HsiangKai updated this revision to Diff 311188.Dec 11 2020, 5:35 AM

clang format.

HsiangKai updated this revision to Diff 311189.Dec 11 2020, 5:45 AM

Avoid a library layering violation.

HsiangKai marked 3 inline comments as done.Dec 11 2020, 5:47 AM
craig.topper added inline comments.Dec 11 2020, 1:25 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
337

Is MVT::i1 needed here? The custom lowering only checks i8/i16/i32.

Should we only have i32 as custom with RV64?

2422

Pass by reference?

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
129

lowerest->lowest

131

This will fail for VMV1R_V won't it?

133

I'm not sure I'm comfortable with losing the tracking of the full register write before all machine IR passes run. Can we have a Pseudo instructions for VMV2/4/8_V with the right register classes and let RISCVMCInstLower.cpp expand them?

craig.topper added inline comments.Dec 11 2020, 1:53 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
231

It's weird that we have a multiclass and a class both named VPseudoBinary. It works because they are considered difference namespaces by tablegen. But it makes the file hard to navigate for a human.

evandro added inline comments.Dec 11 2020, 6:03 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
337

Good question...

HsiangKai added inline comments.Dec 11 2020, 6:15 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
337

MVT::i1 is not needed. We need i8/i16. In vadd.vx, the SEW of scalar argument may be 8 or 16.

HsiangKai updated this revision to Diff 311351.Dec 11 2020, 6:22 PM

Address @craig.topper's comments. I will create pseudo instructions for vector register move later.

HsiangKai marked 5 inline comments as done.Dec 11 2020, 6:25 PM

Define pseudo instructions for vector whole register move.

HsiangKai updated this revision to Diff 311587.Dec 14 2020, 7:24 AM

This looks good to me other than that one comment.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
101

Comment needs to be updated to mention vectors

evandro accepted this revision.Dec 14 2020, 3:47 PM

LGTM, but, please, see if any of the lint notices can be addressed before committing.

This revision is now accepted and ready to land.Dec 14 2020, 3:47 PM

RV32 tests are also needed, please.

HsiangKai updated this revision to Diff 311777.Dec 14 2020, 8:21 PM

Address @craig.topper and @evandro's comments.

This revision was landed with ongoing or failed builds.Dec 14 2020, 9:00 PM
This revision was automatically updated to reflect the committed changes.