Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
18 ↗(On Diff #310804)

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
243–244

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.