This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support inline asm for vector instructions.
ClosedPublic

Authored by HsiangKai on Feb 25 2021, 8:53 AM.

Details

Summary

Types of fractional LMUL and LMUL=1 are all using VR register class. When
using inline asm, it will use the first type in the register class as the
type for the register. It is not necessary the same as the value type. We
need to use INSERT_SUBVECTOR/EXTRACT_SUBVECToR/BITCAST to make it legal
to put the value in the corresponding register class.

Diff Detail

Event Timeline

HsiangKai created this revision.Feb 25 2021, 8:53 AM
HsiangKai requested review of this revision.Feb 25 2021, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 8:53 AM
HsiangKai updated this revision to Diff 326427.Feb 25 2021, 9:37 AM

Convert V0~V31 to the super register if the value type is belong to M2/M4/M8.

HsiangKai updated this revision to Diff 326428.Feb 25 2021, 9:39 AM

Update test cases.

craig.topper added inline comments.Feb 25 2021, 12:13 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6773

ValueVT.getVectorElementCount().getKnownMinValue() is equivalents to ValueVT.getVectorMinNumElements() I think.

Or would ValueVT.getSizeInBits().getKnownMinValue() replace the whole thing?

HsiangKai updated this revision to Diff 326570.Feb 25 2021, 6:05 PM

Address comments.

HsiangKai marked an inline comment as done.Feb 25 2021, 7:54 PM
frasercrmck added inline comments.Feb 26 2021, 2:42 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

In what situations are the element types not equal? Does the PartVT come from the first type in the RC?

6778

Is this guaranteed not to become 0?

6779

Maybe put a /*IsScalable*/ on the true?

llvm/test/CodeGen/RISCV/rvv/inline-asm.ll
3

Nit, but -o - and < %s isn't too common a combination: < %s or -o - %s?

HsiangKai added inline comments.Feb 26 2021, 7:29 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

Yes, the PartVT comes from the first type in the RC. Another way to handle it is to search the corresponding type in the RC in GetRegistersForValue(). The problem comes from we reuse VR register class for fractional LMUL types.

6778

Yes, I reordered the supported types in VR. The first type has the element type i8. In addition, I created VM register class for mask register types.

HsiangKai updated this revision to Diff 326690.Feb 26 2021, 7:34 AM

Address @frasercrmck 's comments.

HsiangKai marked 2 inline comments as done.Feb 26 2021, 7:35 AM
frasercrmck added inline comments.Feb 26 2021, 7:37 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

Thanks! (Un)Luckily I've seen the "first type" issue before: I wish there was a better way.

6778

Cool. Then perhaps we can check/assert on this, just so the types aren't accidentally reordered in the future? This seems to me like quite a few implicit dependencies being used and handed to us via this method.

HsiangKai updated this revision to Diff 326698.Feb 26 2021, 7:54 AM

Add an assertion to check the number of element is not zero.

HsiangKai added inline comments.Feb 26 2021, 8:00 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

@rogfer01 provided another way to deal with the issue in GetRegistersForValue() in their repository.

He searched the types in the register class for the value type. Do you think it is better to do in this way or provide target hooks to deal with it?

@@ -7906,7 +7906,20 @@ static void GetRegistersForValue(SelectionDAG &DAG, const SDLoc &
   // Get the actual register value type.  This is important, because the user
   // may have asked for (e.g.) the AX register in i32 type.  We need to
   // remember that AX is actually i16 to get the right extension.
-  const MVT RegVT = *TRI.legalclasstypes_begin(*RC);
+  MVT RegVT = *TRI.legalclasstypes_begin(*RC);
+  // Try harder for vectors just in case sizes don't match.
+  // FIXME: This is caused by having nxv1i1 in the same register class as
+  // nxv1i64 in EPI.
+  if (RegVT.isVector() && RefOpInfo.ConstraintVT.isVector() &&
+      RegVT.getSizeInBits() != RefOpInfo.ConstraintVT.getSizeInBits()) {
+    auto E = TRI.legalclasstypes_end(*RC);
+    auto I = std::find_if(TRI.legalclasstypes_begin(*RC), E,
+                          [&RefOpInfo](const MVT::SimpleValueType &VTy) {
+                            return MVT(VTy) == RefOpInfo.ConstraintVT;
+                          });
+    if (I != E)
+      RegVT = *I;
+  }
HsiangKai marked 2 inline comments as done.Feb 26 2021, 8:02 AM
frasercrmck added inline comments.Mar 2 2021, 2:07 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6250

I might be missing something: could VM be folded into the loop below?

6777

It's not really specific to vectors, so if we were to go down that route I'd probably expect us to have to offer it up to everything. If I remember correctly, I once had i8,i16,i32,v2i8,v4i8,v2i16 (maybe others) in the same "scalar" register class.

HsiangKai updated this revision to Diff 327641.Mar 2 2021, 6:02 PM

Put VMRegClass before VRRegClass and VRMxRegClass. Handle them in the same loop.

HsiangKai marked an inline comment as done.Mar 2 2021, 6:02 PM
HsiangKai added inline comments.Mar 2 2021, 6:07 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

It seems only RISC-V encountered the problem when choosing the type for the register classes. That is why I provided the target hooks to avoid the problems instead of adding checking in GetRegistersForValue().

If you think it is appropriate to add checking here, I think it is a simpler way to solve the problem.

frasercrmck added inline comments.Mar 4 2021, 1:49 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6777

I think it's fine as you've got it, thanks.

HsiangKai updated this revision to Diff 329164.Mar 8 2021, 4:56 PM

To select the correct types for values in GetRegistersForValue().

frasercrmck added inline comments.Mar 9 2021, 1:50 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8087 ↗(On Diff #329164)

Does this case trigger for any other target?

I'm curious: what made you choose this method in the end?

8088 ↗(On Diff #329164)

These clang-tidy warnings about const auto * look useful

HsiangKai added inline comments.Mar 9 2021, 9:00 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8087 ↗(On Diff #329164)

I prefer to use target hooks to solve the issue.
I think I misunderstood you. I will revert it back.

HsiangKai updated this revision to Diff 329362.Mar 9 2021, 9:12 AM

Revert to use target hooks.

frasercrmck accepted this revision.Mar 10 2021, 2:21 AM

LGTM, though maybe someone else should take a look?

This revision is now accepted and ready to land.Mar 10 2021, 2:21 AM
This revision was automatically updated to reflect the committed changes.