[RISCV][GlobalISel] Legalize scalable vectorized G_ADD, G_SUB, G_AND, G_OR, and G_XOR #71400
Conversation
|
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-llvm-globalisel Author: Jiahan Xie (jiahanxie353) ChangesHi, I'm excited to share thisa draft PR for introducing support for the RISC-V Vector Extension I'm working on legalization and have this super minimum test case. Firstly, I though I want to verify that my test cases themselves are correct. The way I want to verify that they are correct is by using Is there any obvious mistake I made? Any feedback and guidance are greatly appreciated! Thank you in advance for your time and assistance. (I don't how to request reviewers after creating this draft PR so I'll just @ you here @michaelmaitland :) Patch is 43.41 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71400.diff 14 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
index 337fab735a09522..4fb6ba7c26930af 100644
--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
@@ -283,8 +283,8 @@ class TargetRegisterInfo : public MCRegisterInfo {
// DenseMapInfo<unsigned> uses -1u and -2u.
/// Return the size in bits of a register from class RC.
- unsigned getRegSizeInBits(const TargetRegisterClass &RC) const {
- return getRegClassInfo(RC).RegSize;
+ TypeSize getRegSizeInBits(const TargetRegisterClass &RC) const {
+ return TypeSize::Fixed(getRegClassInfo(RC).RegSize);
}
/// Return the size in bytes of the stack slot allocated to hold a spilled
@@ -858,7 +858,7 @@ class TargetRegisterInfo : public MCRegisterInfo {
const TargetRegisterClass *RC) const = 0;
/// Returns size in bits of a phys/virtual/generic register.
- unsigned getRegSizeInBits(Register Reg, const MachineRegisterInfo &MRI) const;
+ TypeSize getRegSizeInBits(Register Reg, const MachineRegisterInfo &MRI) const;
/// Get the weight in units of pressure for this register unit.
virtual unsigned getRegUnitWeight(unsigned RegUnit) const = 0;
diff --git a/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp b/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
index 975787a8f5e734f..2527b1431289677 100644
--- a/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
@@ -358,7 +358,7 @@ static void buildCopyFromRegs(MachineIRBuilder &B, ArrayRef<Register> OrigRegs,
if (PartLLT.isVector() == LLTy.isVector() &&
PartLLT.getScalarSizeInBits() > LLTy.getScalarSizeInBits() &&
(!PartLLT.isVector() ||
- PartLLT.getNumElements() == LLTy.getNumElements()) &&
+ PartLLT.getElementCount() == LLTy.getElementCount()) &&
OrigRegs.size() == 1 && Regs.size() == 1) {
Register SrcReg = Regs[0];
@@ -406,6 +406,7 @@ static void buildCopyFromRegs(MachineIRBuilder &B, ArrayRef<Register> OrigRegs,
// If PartLLT is a mismatched vector in both number of elements and element
// size, e.g. PartLLT == v2s64 and LLTy is v3s32, then first coerce it to
// have the same elt type, i.e. v4s32.
+ // TODO: Extend this coersion to element multiples other than just 2.
if (PartLLT.getSizeInBits() > LLTy.getSizeInBits() &&
PartLLT.getScalarSizeInBits() == LLTy.getScalarSizeInBits() * 2 &&
Regs.size() == 1) {
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 5b4e2b725e1dd76..80e9c08e850b683 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -1065,16 +1065,16 @@ void MachineIRBuilder::validateTruncExt(const LLT DstTy, const LLT SrcTy,
#ifndef NDEBUG
if (DstTy.isVector()) {
assert(SrcTy.isVector() && "mismatched cast between vector and non-vector");
- assert(SrcTy.getNumElements() == DstTy.getNumElements() &&
+ assert(SrcTy.getElementCount() == DstTy.getElementCount() &&
"different number of elements in a trunc/ext");
} else
assert(DstTy.isScalar() && SrcTy.isScalar() && "invalid extend/trunc");
if (IsExtend)
- assert(DstTy.getSizeInBits() > SrcTy.getSizeInBits() &&
+ assert(TypeSize::isKnownGT(DstTy.getSizeInBits(), SrcTy.getSizeInBits()) &&
"invalid narrowing extend");
else
- assert(DstTy.getSizeInBits() < SrcTy.getSizeInBits() &&
+ assert(TypeSize::isKnownLT(DstTy.getSizeInBits(), SrcTy.getSizeInBits()) &&
"invalid widening trunc");
#endif
}
diff --git a/llvm/lib/CodeGen/LowLevelType.cpp b/llvm/lib/CodeGen/LowLevelType.cpp
index 24c30b756737b20..cd85bf606989f9e 100644
--- a/llvm/lib/CodeGen/LowLevelType.cpp
+++ b/llvm/lib/CodeGen/LowLevelType.cpp
@@ -17,7 +17,7 @@ using namespace llvm;
LLT::LLT(MVT VT) {
if (VT.isVector()) {
- bool asVector = VT.getVectorMinNumElements() > 1;
+ bool asVector = VT.getVectorMinNumElements() > 1 || VT.isScalableVector();
init(/*IsPointer=*/false, asVector, /*IsScalar=*/!asVector,
VT.getVectorElementCount(), VT.getVectorElementType().getSizeInBits(),
/*AddressSpace=*/0);
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index dadaf60fa09da04..9ccda9abc019ddf 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -965,7 +965,7 @@ bool MachineVerifier::verifyVectorElementMatch(LLT Ty0, LLT Ty1,
return false;
}
- if (Ty0.isVector() && Ty0.getNumElements() != Ty1.getNumElements()) {
+ if (Ty0.isVector() && Ty0.getElementCount() != Ty1.getElementCount()) {
report("operand types must preserve number of vector elements", MI);
return false;
}
@@ -1937,8 +1937,8 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
// If we have only one valid type, this is likely a copy between a virtual
// and physical register.
- unsigned SrcSize = 0;
- unsigned DstSize = 0;
+ TypeSize SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI);
+ TypeSize DstSize = TRI->getRegSizeInBits(DstReg, *MRI);
if (SrcReg.isPhysical() && DstTy.isValid()) {
const TargetRegisterClass *SrcRC =
TRI->getMinimalPhysRegClassLLT(SrcReg, DstTy);
@@ -1946,7 +1946,7 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
SrcSize = TRI->getRegSizeInBits(*SrcRC);
}
- if (SrcSize == 0)
+ if (SrcSize.isZero())
SrcSize = TRI->getRegSizeInBits(SrcReg, *MRI);
if (DstReg.isPhysical() && SrcTy.isValid()) {
@@ -1956,10 +1956,21 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
DstSize = TRI->getRegSizeInBits(*DstRC);
}
- if (DstSize == 0)
+ if (DstSize.isZero())
DstSize = TRI->getRegSizeInBits(DstReg, *MRI);
- if (SrcSize != 0 && DstSize != 0 && SrcSize != DstSize) {
+ // If the Dst is scalable and the Src is fixed, then the Dst can only hold
+ // the Src if the minimum size Dst can hold is at least as big as Src.
+ if (DstSize.isScalable() && !SrcSize.isScalable() &&
+ DstSize.getKnownMinValue() <= SrcSize.getFixedValue())
+ break;
+ // If the Src is scalable and the Dst is fixed, then Dest can only hold
+ // the Src is known to fit in Dest
+ if (SrcSize.isScalable() && !DstSize.isScalable() &&
+ TypeSize::isKnownLE(DstSize, SrcSize))
+ break;
+
+ if (SrcSize.isNonZero() && DstSize.isNonZero() && SrcSize != DstSize) {
if (!DstOp.getSubReg() && !SrcOp.getSubReg()) {
report("Copy Instruction is illegal with mismatching sizes", MI);
errs() << "Def Size = " << DstSize << ", Src Size = " << SrcSize
diff --git a/llvm/lib/CodeGen/TargetRegisterInfo.cpp b/llvm/lib/CodeGen/TargetRegisterInfo.cpp
index 1bb35f40facfd0f..c50b1cf9422717a 100644
--- a/llvm/lib/CodeGen/TargetRegisterInfo.cpp
+++ b/llvm/lib/CodeGen/TargetRegisterInfo.cpp
@@ -499,7 +499,7 @@ bool TargetRegisterInfo::regmaskSubsetEqual(const uint32_t *mask0,
return true;
}
-unsigned
+TypeSize
TargetRegisterInfo::getRegSizeInBits(Register Reg,
const MachineRegisterInfo &MRI) const {
const TargetRegisterClass *RC{};
@@ -508,16 +508,15 @@ TargetRegisterInfo::getRegSizeInBits(Register Reg,
// Instead, we need to access a register class that contains Reg and
// get the size of that register class.
RC = getMinimalPhysRegClass(Reg);
- } else {
- LLT Ty = MRI.getType(Reg);
- unsigned RegSize = Ty.isValid() ? Ty.getSizeInBits() : 0;
- // If Reg is not a generic register, query the register class to
- // get its size.
- if (RegSize)
- return RegSize;
- // Since Reg is not a generic register, it must have a register class.
- RC = MRI.getRegClass(Reg);
+ assert(RC && "Unable to deduce the register class");
+ return getRegSizeInBits(*RC);
}
+ LLT Ty = MRI.getType(Reg);
+ if (Ty.isValid())
+ return Ty.getSizeInBits();
+
+ // Since Reg is not a generic register, it may have a register class.
+ RC = MRI.getRegClass(Reg);
assert(RC && "Unable to deduce the register class");
return getRegSizeInBits(*RC);
}
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index a1dbc21ca364666..cbcdb54a0fcd2d1 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -14,6 +14,8 @@
#include "RISCVCallLowering.h"
#include "RISCVISelLowering.h"
+#include "RISCVMachineFunctionInfo.h"
+#include "RISCVMachineFunctionInfo.h"
#include "RISCVSubtarget.h"
#include "llvm/CodeGen/Analysis.h"
#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
@@ -185,6 +187,9 @@ struct RISCVIncomingValueAssigner : public CallLowering::IncomingValueAssigner {
const DataLayout &DL = MF.getDataLayout();
const RISCVSubtarget &Subtarget = MF.getSubtarget<RISCVSubtarget>();
+ if (LocVT.isScalableVector())
+ MF.getInfo<RISCVMachineFunctionInfo>()->setIsVectorCall();
+
if (RISCVAssignFn(DL, Subtarget.getTargetABI(), ValNo, ValVT, LocVT,
LocInfo, Flags, State, /*IsFixed=*/true, IsRet, Info.Ty,
*Subtarget.getTargetLowering(),
@@ -301,8 +306,31 @@ struct RISCVCallReturnHandler : public RISCVIncomingValueHandler {
RISCVCallLowering::RISCVCallLowering(const RISCVTargetLowering &TLI)
: CallLowering(&TLI) {}
+/// Return true if scalable vector with ScalarTy is legal for lowering.
+static bool isLegalElementTypeForRVV(Type *EltTy,
+ const RISCVSubtarget &Subtarget) {
+ if (EltTy->isPointerTy())
+ return Subtarget.is64Bit() ? Subtarget.hasVInstructionsI64() : true;
+ if (EltTy->isIntegerTy(1) || EltTy->isIntegerTy(8) ||
+ EltTy->isIntegerTy(16) || EltTy->isIntegerTy(32))
+ return true;
+ if (EltTy->isIntegerTy(64))
+ return Subtarget.hasVInstructionsI64();
+ if (EltTy->isHalfTy())
+ return Subtarget.hasVInstructionsF16();
+ if (EltTy->isBFloatTy())
+ return Subtarget.hasVInstructionsBF16();
+ if (EltTy->isFloatTy())
+ return Subtarget.hasVInstructionsF32();
+ if (EltTy->isDoubleTy())
+ return Subtarget.hasVInstructionsF64();
+ return false;
+}
+
// TODO: Support all argument types.
-static bool isSupportedArgumentType(Type *T, const RISCVSubtarget &Subtarget) {
+// TODO: Remove IsLowerArgs argument by adding support for vectors in lowerCall.
+static bool isSupportedArgumentType(Type *T, const RISCVSubtarget &Subtarget,
+ bool IsLowerArgs = false) {
// TODO: Integers larger than 2*XLen are passed indirectly which is not
// supported yet.
if (T->isIntegerTy())
@@ -311,6 +339,11 @@ static bool isSupportedArgumentType(Type *T, const RISCVSubtarget &Subtarget) {
return true;
if (T->isPointerTy())
return true;
+ // TODO: Support fixed vector types.
+ if (IsLowerArgs && T->isVectorTy() && Subtarget.hasVInstructions() &&
+ T->isScalableTy() &&
+ isLegalElementTypeForRVV(T->getScalarType(), Subtarget))
+ return true;
return false;
}
@@ -398,7 +431,8 @@ bool RISCVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
const RISCVSubtarget &Subtarget =
MIRBuilder.getMF().getSubtarget<RISCVSubtarget>();
for (auto &Arg : F.args()) {
- if (!isSupportedArgumentType(Arg.getType(), Subtarget))
+ if (!isSupportedArgumentType(Arg.getType(), Subtarget,
+ /*IsLowerArgs=*/true))
return false;
}
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll
index 5dd62de8a6bc415..a3a913d8ce02d83 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll
@@ -22,7 +22,7 @@ entry:
ret <vscale x 1 x i8> %a
}
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction{{.*}}scalable_inst
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction: call:
; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_inst
define <vscale x 1 x i8> @scalable_inst(i64 %0) nounwind {
entry:
@@ -35,7 +35,7 @@ entry:
ret <vscale x 1 x i8> %a
}
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction{{.*}}scalable_alloca
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction: alloca:
; FALLBACK-WITH-REPORT-OUT-LABEL: scalable_alloca
define void @scalable_alloca() #1 {
%local0 = alloca <vscale x 16 x i8>
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-bf16-err.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-bf16-err.ll
new file mode 100644
index 000000000000000..f39e7793e5d4f31
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-bf16-err.ll
@@ -0,0 +1,16 @@
+; RUN: not --crash llc -mtriple=riscv32 -mattr=+v -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s 2>&1 | FileCheck %s
+; RUN: not --crash llc -mtriple=riscv64 -mattr=+v -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s 2>&1 | FileCheck %s
+
+; The purpose of this test is to show that the compiler throws an error when
+; there is no support for bf16 vectors. If the compiler did not throw an error,
+; then it will try to scalarize the argument to an s32, which may drop elements.
+define void @test_args_nxv1bf16(<vscale x 1 x bfloat> %a) {
+entry:
+ ret void
+}
+
+; CHECK: LLVM ERROR: unable to lower arguments: ptr (in function: test_args_nxv1bf16)
+
+
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-f16-err.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-f16-err.ll
new file mode 100644
index 000000000000000..042b455bfb54754
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args-f16-err.ll
@@ -0,0 +1,16 @@
+; RUN: not --crash llc -mtriple=riscv32 -mattr=+v -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s 2>&1 | FileCheck %s
+; RUN: not --crash llc -mtriple=riscv64 -mattr=+v -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s 2>&1 | FileCheck %s
+
+; The purpose of this test is to show that the compiler throws an error when
+; there is no support for f16 vectors. If the compiler did not throw an error,
+; then it will try to scalarize the argument to an s32, which may drop elements.
+define void @test_args_nxv1f16(<vscale x 1 x half> %a) {
+entry:
+ ret void
+}
+
+; CHECK: LLVM ERROR: unable to lower arguments: ptr (in function: test_args_nxv1f16)
+
+
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args.ll
new file mode 100644
index 000000000000000..4df0a8f48cc8d0b
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/vec-args.ll
@@ -0,0 +1,909 @@
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+v,+experimental-zvfbfmin,+zvfh -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s | FileCheck -check-prefixes=RV32 %s
+; RUN: llc -mtriple=riscv64 -mattr=+v,+experimental-zvfbfmin,+zvfh -global-isel -stop-after=irtranslator \
+; RUN: -verify-machineinstrs < %s | FileCheck -check-prefixes=RV64 %s
+
+; ==========================================================================
+; ============================= Scalable Types =============================
+; ==========================================================================
+
+define void @test_args_nxv1i8(<vscale x 1 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv1i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 1 x s8>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv1i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 1 x s8>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv2i8(<vscale x 2 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv2i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 2 x s8>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv2i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 2 x s8>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv4i8(<vscale x 4 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv4i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 4 x s8>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv4i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 4 x s8>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv8i8(<vscale x 8 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv8i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 8 x s8>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv8i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 8 x s8>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv16i8(<vscale x 16 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv16i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8m2
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 16 x s8>) = COPY $v8m2
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv16i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8m2
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 16 x s8>) = COPY $v8m2
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv32i8(<vscale x 32 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv32i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8m4
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 32 x s8>) = COPY $v8m4
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv32i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8m4
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 32 x s8>) = COPY $v8m4
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv64i8(<vscale x 64 x i8> %a) {
+ ; RV32-LABEL: name: test_args_nxv64i8
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8m8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 64 x s8>) = COPY $v8m8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv64i8
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8m8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 64 x s8>) = COPY $v8m8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv1i16(<vscale x 1 x i16> %a) {
+ ; RV32-LABEL: name: test_args_nxv1i16
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 1 x s16>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv1i16
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 1 x s16>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define void @test_args_nxv2i16(<vscale x 2 x i16> %a) {
+ ; RV32-LABEL: name: test_args_nxv2i16
+ ; RV32: bb.1.entry:
+ ; RV32-NEXT: liveins: $v8
+ ; RV32-NEXT: {{ $}}
+ ; RV32-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 2 x s16>) = COPY $v8
+ ; RV32-NEXT: PseudoRET
+ ;
+ ; RV64-LABEL: name: test_args_nxv2i16
+ ; RV64: bb.1.entry:
+ ; RV64-NEXT: liveins: $v8
+ ; RV64-NEXT: {{ $}}
+ ; RV64-NEXT: [[COPY:%[0-9]+]]:_(<vscale x 2 x s16>) = COPY $v8
+ ; RV64-NEXT: PseudoRET
+entry:
+ ret void
+}
+
+define...
[truncated]
|
|
My main pet peeve with scalable vectors is that the binops only take two parameters and there is no space for masks resp. predicates today. |
|
Thanks for working on this. I added myself as a reviewer in the top right -- you can click on reviewers and add names. I also added the llvm:globalisel and backend:RISCV-V labels for more visibility. We should add some more reviewers to this once we figure out the problem you are having and once we clean up the commits a bit. It looks like the test is failing because of the following assertion failure when you run it with the options in the RUN line: I touched this function in one of my PRs: #70881. Maybe you can investigate why we're failing this assertion as a result of this test case? Were we supposed to assign a register a regclass? Maybe we need to do some work in RegBankSelection to avoid this failure? I am not sure why the failure happens with the options you give it. However, I will note that SDAG works on LLVM IR, not MIR so I don't think you'd really get the result you intended on getting anyway. |
|
Are you able to reorder your commits with We should get the commit history to look like this: Once it looks like this, you can force push to this branch. This will give reviewers an easy way to review the changes introduced in your commit. Then you can push additional commits to the branch as you respond to review comments. You can see an example of this here: https://github.com/llvm/llvm-project/pull/70882/commits. |
I don't think the |
|
The title hints at that he wants to legalize vadd for RISC-V in GiSel. I would expect to see changes to: |
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
a20ba10
to
a029a29
Compare
Does it look right now? |
Yes, thanks! |
| @@ -0,0 +1,9 @@ | |||
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py | |||
There was a problem hiding this comment.
Maybe we should append vector add cases to legalize-add.mir file instead of creating a new one. It looks like that legalize-add.mir file is testing G_ADD leglization on different types.
There was a problem hiding this comment.
Also because vadd is not the GISel opcode we are legalize. We are legalizing a G_ADD opcode that operates on vector types.
There was a problem hiding this comment.
Sounds reasonable! And I tried to change it, and used SelectionDAG to run. But again, it fails at
/llvm-project/llvm/include/llvm/CodeGen/MachineRegisterInfo.h:665: const TargetRegisterClass *llvm::MachineRegisterInfo::getRegClass(Register) const: Assertion `isa<const TargetRegisterClass *>(VRegInfo[Reg.id()].first) && "Register class not set, wrong accessor"' failed.
Nevertheless, as you comment before:
I touched this function in one of my PRs: #70881. Maybe you can investigate why we're failing this assertion as a result of this test case? Were we supposed to assign a register a regclass? Maybe we need to do some work in RegBankSelection to avoid this failure?
I am not sure why the failure happens with the options you give it. However, I will note that SDAG works on LLVM IR, not MIR so I don't think you'd really get the result you intended on getting anyway.
Let me just proceed with -global-isel and see if it will fail at the same spot. If it does, I will take a closer look at it; for now, I'll just ignore SelectionDAG's error since it's an MIR test and that
I will note that SDAG works on LLVM IR, not MIR so I don't think you'd really get the result you intended on getting anyway
What do you think
| ; RUN: llc -mtriple=riscv32 -mattr=+v -stop-after=irtranslator | FileCheck %s --check-prefixes=CHECK,RV32 | ||
| ; RUN: llc -mtriple=riscv64 -mattr=+v --stop-after=irtranslator | FileCheck %s --check-prefixes=CHECK,RV64 | ||
|
|
||
| define <vscale x 1 x i8> @vadd_vv_mask_nxv1i8(<vscale x 1 x i8> %va, <vscale x 1 x i8> %vb, <vscale x 1 x i1> %mask) { |
There was a problem hiding this comment.
If I understand correctly, you'd like to test the code generation for the %vc = add <vscale x 1 x i8> %va, %vs here. Can you accomplish that with the following simplification:
define <vscale x 1 x i8> add_nxv1i8(<vscale x 1 x i8> %a, <vscale x 1 x i8> %b) {
%c = add <vscale x 1 x i8> %a, %b
ret <vscale x 1 x i8> %c
}There was a problem hiding this comment.
Yes, no problem!
Can you point me to the RVV calling convention? I'd like to have my CHECK statements conform to the calling convention. For this simplified example you gave, is it correct if I have:
; CHECK-LABEL: add_nxv1i8:
; CHECK: # %bb.0:
; CHECK-NEXT: vadd.vv v10, v8, v9
; CHECK-NEXT: ret
Especially, how should I write ; CHECK-NEXT: vadd.vv v10, v8, v9
There was a problem hiding this comment.
I usually generate test checks using update_mir_test_checks or update_llc_test_checks. Something like this:
llvm/utils/update_mir_test_checks.py --llc-binary=build/bin/llc llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/my-test.ll
In your case, the compiler may return an error when you try to use the update_test_checks scripts since you haven't implemented the functionality yet. I usually don't worry about having CHECKs before the functionality is implemented. It is a good idea to know roughly what code you want the test case to generate before you implement it. You can get an idea by seeing what SDAG does.build/bin/llc -mtriple=riscv32 -mattr=+v mytest.ll gives something like:
add_nxv1i8:
vsetvli a0, zero, e8, mf8, ta, ma
vadd.vv v8, v8, v9
ret
riscv-non-isa/riscv-elf-psabi-doc#389 has discussion on a formal ABI for RVV. The proposal is currently implemented in LLVM. I wouldn't worry too much about it in the scope of this patch since thats really the job of lowerFormalArguments and lowerReturnVal to get right.
This PR might give you a good idea what tests should look like for a legalizer patch. It is unlikely that you will be able to add a test that goes from LLVM IR -> RISC-V ASM in this PR since other gisel-passes may need some work. For example, if you run that test case with -global-isel, you will see that it currently fails in IRTranslator to convert the LLVM add to G_ADD. Once you implement that functionality, you might see it fail in the legalizer, and once you fix that, you might see it fail in instruction-select.
There was a problem hiding this comment.
Thanks for explaining!
Just wanted to clarify:
if you run that test case with -global-isel, you will see that it currently fails in IRTranslator to convert the LLVM add to G_ADD. Once you implement that functionality, you might see it fail in the legalizer
So it currently fails at here because #70133 patch made it fall back to SelectionDAG. I tried to comment it out and keep it running, it fails at translate return with vector types.
I was wondering, @michaelmaitland , should I be working on lowering return or you will do it and I'll just focus on legalizing for now?
There was a problem hiding this comment.
I plan on taking a look at lowerReturnVal.
That |
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
a029a29
to
137e9a3
Compare
|
Hi all, I have a simple test case that involve scalable vector n x 1 x 32 legalization and get it passed/legalized. If it looks good, I can move on to support different grouping number combined with different element types/lengths. Since according to the RISCV V Extension Spec, integer Issue
What do you think to tackle with fractional |
If you could look into this, that would be nice. Do you know what the type should be? Why isn't it
You should test types that result in all the different LMULs. If you have tests for the relevant LLVM types, you end up testing all the LMUL values. It would be great if you can add tests for all the types we want to legalize, and support legalization of those types. |
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
137e9a3
to
24c9403
Compare
legalize-vadd.mir
Outdated
| @@ -0,0 +1,58 @@ | |||
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py | |||
There was a problem hiding this comment.
File in wrong directory? Should we put tests for vector G_ADD in the existing tests for legalizing G_ADD?
There was a problem hiding this comment.
I think you can put a copy of each test case in each file:
There was a problem hiding this comment.
This file should give you a good idea which types we'd like to legalize.
There was a problem hiding this comment.
I think you can put a copy of each test case in each file:
sounds good!
This is irrelevant to my test cases, but is it even possible to pass i7, like in here?
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
24c9403
to
24c6848
Compare
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
24c6848
to
0e7ef4b
Compare
jiahanxie353
changed the title
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
0e7ef4b
to
dc1a45b
Compare
That's weird because I just re-ran |
|
It is because %s is passed twice to FileCheck by mistake |
oh I see. Does it matter if we use
|
It does not matter |
jiahanxie353
force-pushed
the
riscv-v-add-gisel
branch
from
b8c9404
to
060edbc
Compare
|
Can you please update PR description before commit? |
Sure! Should it be a one-line description? |
|
It can be multiple lines. It should summarize the changes in this PR. Dont worry about squashing into one, that will happen when we click "Squash and Merge" |
|
The title off this PR is a little long for a commit title. |
|
How about “Legalize scalable vectorized G_ADD, G_SUB, G_AND, G_OR, and G_XOR” |
|
That sounds reasonable to me. |
jiahanxie353
changed the title
|
Is there anything I need to do to make this patch merge into main? |
|
Can you update PR description, as that will be used for commit body? |
I thought I have already updated it to "[RISCV][GlobalISel] Legalize scalable vectorized G_ADD, G_SUB, G_AND, G_OR, and G_XOR" |
|
PR description means your first comment, e.g. |
It should be different from the title, providing some more information to the reader. Here is some more info. https://llvm.org/docs/DeveloperPolicy.html#commit-messages |
|
Does the description look alright now? |
|
Committed in e42a70a |
| @@ -0,0 +1,53 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py | |||
| ; RUN: llc -mtriple=riscv32 -mattr=+v -global-isel -stop-before=legalizer -simplify-mir < %s | FileCheck %s --check-prefixes=CHECK,RV32I | |||
There was a problem hiding this comment.
RV32I and RV64I are unused in this test. This causes FileCheck to report an error in lit. I fixed it, but you should always run ninja check even after running the script. The script isn't perfect.
There was a problem hiding this comment.
Thanks for fixing this. The failure didn't show up in the pre commit CI checks.
Support IR translate for ADD, SUB, AND, OR, and XOR instructions operating on scalable vectors.
And legalize scalable vectorized G_ADD, G_SUB, G_AND, G_OR, and G_XOR for the RISC-V vector extension.