[RISCV][GISel] RegBank select and instruction select for vector G_ADD, G_SUB #74114

jiahanxie353 · 2023-12-01T17:35:05Z

RegisterBank Selection for scalable vector G_ADD and G_SUB by creating new mappings for different types of vector register banks.
Then implement Instruction Selection for the same operations by choosing the correct RISC-V vector register class.

github-actions · 2023-12-01T17:37:29Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jiahanxie353 · 2023-12-01T17:42:14Z

okay, there's nothing showing up under "Reviewers" to request your review, so I'll just @michaelmaitland @topperc

topperc · 2023-12-01T18:26:05Z

llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp

@@ -240,6 +254,10 @@ RISCVRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
      &RISCV::ValueMappings[GPRSize == 64 ? RISCV::GPRB64Idx
                                          : RISCV::GPRB32Idx];

+  unsigned VRBSize = getMaximumSize(RISCV::VRBRegBankID);
+  const ValueMapping *VRBValueMapping =
+      &RISCV::ValueMappings[VRBSize == 64 ? RISCV::VRB64Idx : RISCV::VRB32Idx];


The size for VRB bank shouldn't change. We only have to do this for GPR because of RV32 vs RV64.

for VRB, do we always have size == 64?

It looks like in build/lib/Target/RISCV/RISCVGenRegisterBank.inc that the Sizes array has 512 for both RV32 and RV64 for VRB.

512 is the size of LMUL=8. we should have 4 different sizes in the mapping for the 4 different whole LMUL register sizes. We should pick based on the known minimum size of the type. 512, 256, 128, and 64. Anything less than 64 should use the 64.

Actually I'm a bit lost right now. Can I check my understanding?

The size for VRB bank shouldn't change.

Does this refer to unsigned VRBSize never change?

Based on my understanding of your description, in PartMappings, I'll probably have:

{0, 64, VRBRegBank}, // <= 64 {0, 64, VRBRegBank}, {64, 128, VRBRegBank}, // 128, LMUL=2 {0, 64, VRBRegBank}, {64, 128, VRBRegBank}, {128, 192, VRBRegBank}, {192, 256, VRBRegBank}, // 256, LMUL=4 {0, 64, VRBRegBank} .... // 512, LMUL=8

?
And that I'll only have PMI_VRB64 in PartialMappingIdx since the size of VRB doesn't change?
And in ValueMapping, I'll probably have:

{&PartMappings[PMI_VRB64], 1}, {&PartMappings[PMI_VB64], 2}, {&PartMappings[PMI_VRB64], 4}, {&PartMappings[PMI_VRB64], 8}

?

getMaximumSize(RISCV::VRBRegBankID) will always return the same value. I don't know what that value is. It's probably 512?

PartialMappingIdx should be

enum PartialMappingIdx { PMI_GPRB32 = 0, PMI_GPRB64 = 1, PMI_FPRB32 = 2, PMI_FPRB64 = 3, PMI_VRB64 = 4, PMI_VRB128 = 5, PMI_VRB256 = 6, PMI_VRB512 = 7, };

PartMappings should be

const RegisterBankInfo::PartialMapping PartMappings[] = { {0, 32, GPRBRegBank}, {0, 64, GPRBRegBank}, {0, 32, FPRBRegBank}, {0, 64, FPRBRegBank}, {0, 64, VRBRegBank}, {0, 128, VRBRegBank}, {0, 256, VRBRegBank}, {0, 512, VRBRegBank}, };

ValueMappings should be

const RegisterBankInfo::ValueMapping ValueMappings[] = { // Invalid value mapping. {nullptr, 0}, // Maximum 3 GPR operands; 32 bit. {&PartMappings[PMI_GPRB32], 1}, {&PartMappings[PMI_GPRB32], 1}, {&PartMappings[PMI_GPRB32], 1}, // Maximum 3 GPR operands; 64 bit. {&PartMappings[PMI_GPRB64], 1}, {&PartMappings[PMI_GPRB64], 1}, {&PartMappings[PMI_GPRB64], 1}, // Maximum 3 FPR operands; 32 bit. {&PartMappings[PMI_FPRB32], 1}, {&PartMappings[PMI_FPRB32], 1}, {&PartMappings[PMI_FPRB32], 1}, // Maximum 3 FPR operands; 64 bit. {&PartMappings[PMI_FPRB64], 1}, {&PartMappings[PMI_FPRB64], 1}, {&PartMappings[PMI_FPRB64], 1}, // Maximum 3 VR LMUL=1 operands; {&PartMappings[PMI_VRB64], 1}, {&PartMappings[PMI_VRB64], 1}, {&PartMappings[PMI_VRB64], 1}, // Maximum 3 VR LMUL=2 operands; {&PartMappings[PMI_VRB128], 1}, {&PartMappings[PMI_VRB128], 1}, {&PartMappings[PMI_VRB128], 1}, // Maximum 3 VR LMUL=4 operands; {&PartMappings[PMI_VRB256], 1}, {&PartMappings[PMI_VRB256], 1}, {&PartMappings[PMI_VRB256], 1}, // Maximum 3 VR LMUL=8 operands; {&PartMappings[PMI_VRB512], 1}, {&PartMappings[PMI_VRB512], 1}, {&PartMappings[PMI_VRB512], 1}, };

ValueMappingIdx should be

enum ValueMappingIdx { InvalidIdx = 0, GPRB32Idx = 1, GPRB64Idx = 4, FPRB32Idx = 7, FPRB64Idx = 10, VPRB64Idx = 13, VPRB128Idx = 16, VPRB256Idx = 19, VPRB512Idx = 22, };

You'll need a function like getFPValueMapping to pick the correct VRB*Idx from the minimum type size. If the size is <=64 pick VPRB64Idx, if the size is 128 pick VPRB128Idx, if the size is 256 pick VPRB256Idx, and if the size is 512 VPRB512Idx.

llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp

llvmbot · 2023-12-01T18:39:01Z

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-globalisel

Author: Jiahan Xie (jiahanxie353)

Changes

Hi folks,
I'm starting to work on instruction selection for G_ADD, G_SUB in the vector case.
I have selected vector register bank for vector cases.

Now I'm moving on to the select function.
My test case is as simple as follows:

# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv32 -mattr=+v -run-pass=instruction-select \
# RUN:   -simplify-mir -verify-machineinstrs %s \
# RUN:   -o - | FileCheck -check-prefix=RV32I %s

---
name:            add_nxv1s32
legalized:       true
regBankSelected: true
tracksRegLiveness: true
body:             |
  bb.0.entry:
    liveins: $v10, $v11

    %0:vrb(&lt;vscale x 1 x s32&gt;) = COPY $v10
    %1:vrb(&lt;vscale x 1 x s32&gt;) = COPY $v11
    %2:vrb(&lt;vscale x 1 x s32&gt;) = G_ADD %0, %1
    $v10 = COPY %2(&lt;vscale x 1 x s32&gt;)
    PseudoRET implicit $v10

...

However, when it's trying to select G_ADD, this if statement holds true so it never reaches selectImpl.
Do you have any idea why? It says it does not have the property of being a PreISelOpCode. What is a PreISelOpCode, and why isn't vector G_ADD has this property so it has to go to "unusual legalization steps" (as the documentation of this function says:)
> /// Return true if this is an instruction that should go through the usual legalization steps.

And I'm a bit confused about all the td files, such as RISCVGISel.td, RISCVInstrInfo.td, RISCVInstrInfoV.td, and RISCVInstrInfoV.td.
In order for TableGen to pick up vector G_ADD, which td file should I modify?

Full diff: https://github.com/llvm/llvm-project/pull/74114.diff

1 Files Affected:

(modified) llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp (+21)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
index cf0ff63a5e51c29..f3be5ba74b2f313 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
@@ -29,6 +29,8 @@ const RegisterBankInfo::PartialMapping PartMappings[] = {
     {0, 64, GPRBRegBank},
     {0, 32, FPRBRegBank},
     {0, 64, FPRBRegBank},
+    {0, 32, VRBRegBank},
+    {0, 64, VRBRegBank},
 };
 
 enum PartialMappingIdx {
@@ -36,6 +38,8 @@ enum PartialMappingIdx {
   PMI_GPRB64 = 1,
   PMI_FPRB32 = 2,
   PMI_FPRB64 = 3,
+  PMI_VRB32 = 4,
+  PMI_VRB64 = 5,
 };
 
 const RegisterBankInfo::ValueMapping ValueMappings[] = {
@@ -57,6 +61,14 @@ const RegisterBankInfo::ValueMapping ValueMappings[] = {
     {&PartMappings[PMI_FPRB64], 1},
     {&PartMappings[PMI_FPRB64], 1},
     {&PartMappings[PMI_FPRB64], 1},
+    // Maximum 3 VRB operands; 32 bit.
+    {&PartMappings[PMI_VRB32], 1},
+    {&PartMappings[PMI_VRB32], 1},
+    {&PartMappings[PMI_VRB32], 1},
+    // Maximum 3 VRB operands; 64 bit.
+    {&PartMappings[PMI_VRB64], 1},
+    {&PartMappings[PMI_VRB64], 1},
+    {&PartMappings[PMI_VRB64], 1},
 };
 
 enum ValueMappingIdx {
@@ -65,6 +77,8 @@ enum ValueMappingIdx {
   GPRB64Idx = 4,
   FPRB32Idx = 7,
   FPRB64Idx = 10,
+  VRB32Idx = 13,
+  VRB64Idx = 16,
 };
 } // namespace RISCV
 } // namespace llvm
@@ -240,6 +254,10 @@ RISCVRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
       &RISCV::ValueMappings[GPRSize == 64 ? RISCV::GPRB64Idx
                                           : RISCV::GPRB32Idx];
 
+  unsigned VRBSize = getMaximumSize(RISCV::VRBRegBankID);
+  const ValueMapping *VRBValueMapping =
+      &RISCV::ValueMappings[VRBSize == 64 ? RISCV::VRB64Idx : RISCV::VRB32Idx];
+
   switch (Opc) {
   case TargetOpcode::G_ADD:
   case TargetOpcode::G_SUB:
@@ -269,6 +287,9 @@ RISCVRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
   case TargetOpcode::G_ZEXT:
   case TargetOpcode::G_SEXTLOAD:
   case TargetOpcode::G_ZEXTLOAD:
+    if (MRI.getType(MI.getOperand(0).getReg()).isVector())
+      return getInstructionMapping(DefaultMappingID, /*Cost=*/1,
+                                   VRBValueMapping, NumOperands);
     return getInstructionMapping(DefaultMappingID, /*Cost=*/1, GPRValueMapping,
                                  NumOperands);
   case TargetOpcode::G_FADD:

topperc · 2023-12-01T18:42:40Z

You're missing changes to RISCVInstructionSelector::getRegClassForTypeOnBank

michaelmaitland · 2023-12-01T18:56:14Z

I checked out your code and I am not seeing early exit before selectImpl for the G_ADD. I only see the COPY and PseudoRET instructions entering into the if (!MI.isPreISelOpcode() || Opc == TargetOpcode::G_PHI) {...} block.

jiahanxie353 · 2023-12-01T21:14:02Z

I checked out your code and I am not seeing early exit before selectImpl for the G_ADD. I only see the COPY and PseudoRET instructions entering into the if (!MI.isPreISelOpcode() || Opc == TargetOpcode::G_PHI) {...} block.

did you pick the vector legalizer commits from my other PR?

michaelmaitland · 2023-12-01T21:23:28Z

I checked out your code and I am not seeing early exit before selectImpl for the G_ADD. I only see the COPY and PseudoRET instructions entering into the if (!MI.isPreISelOpcode() || Opc == TargetOpcode::G_PHI) {...} block.

did you pick the vector legalizer commits from my other PR?

Yes

jiahanxie353 · 2023-12-01T21:44:38Z

I checked out your code and I am not seeing early exit before selectImpl for the G_ADD. I only see the COPY and PseudoRET instructions entering into the if (!MI.isPreISelOpcode() || Opc == TargetOpcode::G_PHI) {...} block.

did you pick the vector legalizer commits from my other PR?

Yes

you are right, I was confused by the backward order

jiahanxie353 · 2023-12-05T13:25:16Z

Hi,
I'm stuck on this part, where it tries to compare Ty.getSizeInBits(), which is a vscale x s32 and TRI.getRegSizeInBits(*RC), which is an int 64.
It seems like there is no > overloaded to compare these two types.
Is this issue expected?

michaelmaitland · 2023-12-05T15:53:25Z

Hi, I'm stuck on this part, where it tries to compare Ty.getSizeInBits(), which is a vscale x s32 and TRI.getRegSizeInBits(*RC), which is an int 64. It seems like there is no > overloaded to compare these two types. Is this issue expected?

Before the addition of scalable vectors, the size of scalars and fixed vectors is always known. For example an i32 is 32 bits, a float is 32 bits, and 4 x i32 is 4 * 32 bits. With the addition of scalable vectors, we do not know the size at compile time. The size of a type like vscale x 1 x i32 depends on the value of vscale, which is a runtime constant, which makes the number of bits of that type unknown at compile time.

Instead of returning unsigned for the size of the type, we must return TypeSize, which is capable of representing that the size is scalable. Take a look at this patch which I added in order to make lowerFormalArguments work. You will likely need to make some unsigned -> TypeSize changes. For the exact case you show here, you probably want to take advantage of the TypeSize::isKnownLT function.

jiahanxie353 · 2023-12-05T16:41:18Z

Hi, I'm stuck on this part, where it tries to compare Ty.getSizeInBits(), which is a vscale x s32 and TRI.getRegSizeInBits(*RC), which is an int 64. It seems like there is no > overloaded to compare these two types. Is this issue expected?

Before the addition of scalable vectors, the size of scalars and fixed vectors is always known. For example an i32 is 32 bits, a float is 32 bits, and 4 x i32 is 4 * 32 bits. With the addition of scalable vectors, we do not know the size at compile time. The size of a type like vscale x 1 x i32 depends on the value of vscale, which is a runtime constant, which makes the number of bits of that type unknown at compile time.

Instead of returning unsigned for the size of the type, we must return TypeSize, which is capable of representing that the size is scalable. Take a look at this patch which I added in order to make lowerFormalArguments work. You will likely need to make some unsigned -> TypeSize changes. For the exact case you show here, you probably want to take advantage of the TypeSize::isKnownLT function.

Thanks!
Seems like the link to the lowerFormalArguments patch is not working? Can you send again?

michaelmaitland · 2023-12-05T16:47:40Z

Thanks! Seems like the link to the lowerFormalArguments patch is not working? Can you send again?

ac4ff61
f219e03
a7bbcc4

jiahanxie353 · 2023-12-06T14:58:29Z

To implement RISCVInstructionSelector::getRegClassForTypeOnBank, I'm creating a mapping function that takes in an LLT and output RegClass.

Let's take vscale x 1 x s32 as an example LLT, the expected RegClass is RISCV::VRRegClassID (the reason being we treat LMUL <= 1 all as LMUL=1 since they have similar behavior, if I remember our discussion from yesterday correctly? @michaelmaitland ).

I was trying to use getElementType to extract s32, and using getNumElements() to extract that 1, which raises an exception. And I believe it's because getNumElements() extracts vscale and 1 at the same time. Since vscale is unknown at compile time, so it can't calculate the exact NumElements.

I'm wondering if I'm on the right track, and is there a generic function to extract n out of vscale x n x m?

michaelmaitland · 2023-12-06T17:04:26Z

is there a generic function to extract n out of vscale x n x m?

https://llvm.org/doxygen/classllvm_1_1details_1_1FixedOrScalableQuantity.html#ac4ab9dd9440c55bee1aa4a1195cee759

jiahanxie353 · 2023-12-08T13:55:07Z

Seems like I don't need to implement all the ValueMapping for some reason, is this because selectImpl is working out-of-the-box?

The test results here looks reasonable to me, but I don't understand the -1, 3 /* e8 */, 3 part in ; RV32I-NEXT: [[PseudoVADD_VV_MF8_:%[0-9]+]]:vr = PseudoVADD_VV_MF8 [[DEF]], [[COPY]], [[COPY1]], -1, 3 /* e8 */, 3 /* ta, ma */.

Please let me know what you think

michaelmaitland · 2023-12-08T17:21:42Z

I don't understand the -1, 3 /* e8 /, 3 part in ; RV32I-NEXT: [[PseudoVADD_VV_MF8_:%[0-9]+]]:vr = PseudoVADD_VV_MF8 [[DEF]], [[COPY]], [[COPY1]], -1, 3 / e8 /, 3 / ta, ma */.

The VPseudoBinaryNoMaskTU class in RISCVInstrInfoVPseudos.td specifies a list of ins operands. There are operands sew and policy which are immediate values. the 3 /* e8 */ is the sew operand and 3 /* ta, ma */ is the policy operand. 3 is the log2 encoding for e8. 3 is the encoding for a ta, ma policy. The comments are added by the emitter to help the reader since they may not have memorized the encodings.

michaelmaitland · 2023-12-08T17:30:46Z

Seems like I don't need to implement all the ValueMapping for some reason

It worked because the test cases have regBankSelected: true and have vrb filled in for the vector instructions:

%2:vrb(<vscale x 1 x s8>) = G_ADD %0, %1
   ^^^

If you run the full gisel pipeline on a llvm program or -run-pass=instruction-select on MIR that just went through the IR translator, regbankselection will fail.

topperc · 2023-12-08T18:18:22Z

I don't understand the -1, 3 /* e8 */, 3 part in ; RV32I-NEXT: [[PseudoVADD_VV_MF8_:%[0-9]+]]:vr = PseudoVADD_VV_MF8 [[DEF]], [[COPY]], [[COPY1]], -1, 3 /* e8 */, 3 /* ta, ma */.

The -1 is the VL operand where -1 means VLMAX.

llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp

jiahanxie353 · 2023-12-11T15:18:38Z

What is the instruction-select supposed to generate? Is it supposed to stop and generate vector instructions pseudos, like what I have right now in the test files; or do we want actual RISC-V vsetvli and vadd instructions after the instruction-select pass?
I think it's the former case and the transformation from pseudos to actual assembly instructions is taken care of by the rest of llvm infrastructure and we don't need to explicitly support in GISel?

michaelmaitland · 2023-12-11T16:17:09Z

I think it's the former case and the transformation from pseudos to actual assembly instructions is taken care of by the rest of llvm infrastructure and we don't need to explicitly support in GISel?

This.

Instruction selection is responsible for lowering into target specific MIR (MachineInstrs). The RISCVInsertVSETVLI pass comes later in the pipeline. There is a RISCVAsmPrinter pass that will convert MachineInstr to MCInst (RISCVAsmPrinter::lowerToMCInst) which is the representation of assembly.

michaelmaitland · 2023-12-11T16:44:35Z

llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp

@@ -57,6 +60,22 @@ const RegisterBankInfo::ValueMapping ValueMappings[] = {
    {&PartMappings[PMI_FPRB64], 1},
    {&PartMappings[PMI_FPRB64], 1},
    {&PartMappings[PMI_FPRB64], 1},
+    // Maximum 3 VR LMUL=1 operands.


Suggested change

// Maximum 3 VR LMUL=1 operands.

// Maximum 3 VR LMUL={1, MF2, MF4, MF8} operands.

michaelmaitland · 2023-12-11T16:52:15Z

llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/add-rv32.mir

@@ -0,0 +1,556 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv32 -mattr=+v -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - \


I think that the test cases are the same for rv32 and rv64. Should we collapse to rvv/add.mir and have a run line for rv32 and a run line for rv64?

# RUN: llc -mtriple=riscv32 -mattr=+v -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - | FileCheck -check-prefix=RV32 %s # RUN: llc -mtriple=riscv64 -mattr=+v -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - | FileCheck -check-prefix=RV64 %s

michaelmaitland · 2023-12-11T16:58:30Z

llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/add-rv32.mir

+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $x10, $x11


x10 and x11 are scalar registers. You should pass $vN registers. Remember that LMUL 1 and fractional LMUL cases use one vector register, LMUL 2 cases use 2 vector registers and so on:

# LMUL 1 liveins: liveins: $v8, $v9 # LMUL 2 liveins: liveins: $v8, $v10 # LMUL 4 liveins: liveins: $v8, $v12, $v16 # LMUL 8 liveins: liveins: $v8, $v16, $v24

michaelmaitland · 2023-12-11T16:58:55Z

llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/add-rv32.mir

+    %0:vrb(<vscale x 1 x s8>) = COPY $x10
+    %1:vrb(<vscale x 1 x s8>) = COPY $x11
+    %2:vrb(<vscale x 1 x s8>) = G_ADD %0, %1
+    $x10 = COPY %2(<vscale x 1 x s8>)


Make sure to copy to vector register, not scalar register.

michaelmaitland · 2023-12-11T17:03:13Z

llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/vec-add-sub-rv32.mir

+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $v10, $v11


Be careful which vector registers you are passing as livein. M8 operations will be using v10, v11, v12, v13, v14, v15, v16, v17 as a single register grouping. That means that you probably should use the next available register for the next live in.

I am calling out this specific LMUL 8 case in this test, but make sure this holds for all your test cases for their respective LMULs. It could be helpful to see which registers the IR translator for a test that lowers to this MIR test case chooses if you are not sure.

Be careful which vector registers you are passing as livein. M8 operations will be using v10, v11, v12, v13, v14, v15, v16, v17 as a single register grouping. That means that you probably should use the next available register for the next live in.

I am calling out this specific LMUL 8 case in this test, but make sure this holds for all your test cases for their respective LMULs. It could be helpful to see which registers the IR translator for a test that lowers to this MIR test case chooses if you are not sure.

Why hand construct the test at all? Write the IR test and use -stop-before to get the MIR.

michaelmaitland · 2023-12-15T14:46:07Z

llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp

@@ -277,7 +277,7 @@ bool InstructionSelect::runOnMachineFunction(MachineFunction &MF) {
    }

    const LLT Ty = MRI.getType(VReg);
-    if (Ty.isValid() && Ty.getSizeInBits() > TRI.getRegSizeInBits(*RC)) {
+    if (Ty.isValid() && TypeSize::isKnownGT(Ty.getSizeInBits(), TRI.getRegSizeInBits(*RC))) {


Clang format needed here?

michaelmaitland

can you mention regbankselect in the title/description of PR?

topperc · 2023-12-15T20:22:30Z

llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp

@@ -785,6 +785,21 @@ const TargetRegisterClass *RISCVInstructionSelector::getRegClassForTypeOnBank(
  }

  // TODO: Non-GPR register classes.


I think we can delete this TODO

topperc · 2023-12-15T20:23:00Z

llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp

-    {0, 64, GPRBRegBank},
-    {0, 32, FPRBRegBank},
-    {0, 64, FPRBRegBank},
+    {0, 32, GPRBRegBank}, {0, 64, GPRBRegBank}, {0, 32, FPRBRegBank},


Please put these back on single lines regardless of what clang-format says

michaelmaitland · 2024-01-16T18:49:53Z

This patch could use a rebase and an update to the PR description.

…vector add and sub

michaelmaitland

LGTM with nit comment that RISCV -> RISC-V in commit description.

* llvm/main: (500 commits) [docs] Add beginner-focused office hours (llvm#80308) [mlir][sparse] external entry method wrapper for sparse tensors (llvm#80326) [StackSlotColoring] Ignore non-spill objects in RemoveDeadStores. (llvm#80242) [libc][stdbit] fix return types (llvm#80337) Revert "[RISCV] Refine cost on Min/Max reduction" (llvm#80340) [TTI]Add support for strided loads/stores. [analyzer][HTMLRewriter] Cache partial rewrite results. (llvm#80220) [flang][openacc][openmp] Use #0 from hlfir.declare value when generating bound ops (llvm#80317) [AArch64][PAC] Expand blend(reg, imm) operation in aarch64-pauth pass (llvm#74729) [SHT_LLVM_BB_ADDR_MAP][llvm-readobj] Implements llvm-readobj handling for PGOAnalysisMap. (llvm#79520) [libc] add bazel support for most of unistd (llvm#80078) [clang-tidy] Remove enforcement of rule C.48 from cppcoreguidelines-prefer-member-init (llvm#80330) [OpenMP] Fix typo (NFC) (llvm#80332) [BOLT] Enable re-writing of Linux kernel binary (llvm#80228) [BOLT] Adjust section sizes based on file offsets (llvm#80226) [libc] fix stdbit include test when not all entrypoints are available (llvm#80323) [RISCV][GISel] RegBank select and instruction select for vector G_ADD, G_SUB (llvm#74114) [RISCV] Add srmcfg CSR from Ssqosid extension. (llvm#79914) [mlir][sparse] add sparsification options to pretty print and debug s… (llvm#80205) [RISCV][MC] MC layer support for the experimental zalasr extension (llvm#79911) ...

…, G_SUB (llvm#74114) RegisterBank Selection for scalable vector G_ADD and G_SUB by creating new mappings for different types of vector register banks. Then implement Instruction Selection for the same operations by choosing the correct RISC-V vector register class.

topperc reviewed Dec 1, 2023

View reviewed changes

michaelmaitland reviewed Dec 1, 2023

View reviewed changes

llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp Outdated Show resolved Hide resolved

michaelmaitland added backend:RISC-V llvm:globalisel labels Dec 1, 2023

jiahanxie353 force-pushed the instr-sel branch from 0a2f5de to d797bfa Compare December 8, 2023 13:51

topperc reviewed Dec 8, 2023

View reviewed changes

llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp Outdated Show resolved Hide resolved

jiahanxie353 force-pushed the instr-sel branch from d797bfa to 04b558b Compare December 10, 2023 14:12

michaelmaitland reviewed Dec 11, 2023

View reviewed changes

jiahanxie353 force-pushed the instr-sel branch from 04b558b to 14db7ee Compare December 12, 2023 17:43

michaelmaitland reviewed Dec 15, 2023

View reviewed changes

jiahanxie353 changed the title ~~[RISCV][GISel] Instruction select for vector G_ADD, G_SUB~~ [RISCV][GISel] RegBank select and instruction select for vector G_ADD, G_SUB Dec 15, 2023

jiahanxie353 force-pushed the instr-sel branch from 14db7ee to 4114266 Compare December 15, 2023 17:49

topperc reviewed Dec 15, 2023

View reviewed changes

jiahanxie353 force-pushed the instr-sel branch 2 times, most recently from 52b9430 to 66db262 Compare January 12, 2024 00:55

michaelmaitland marked this pull request as ready for review January 17, 2024 00:02

jiahanxie353 added 12 commits January 31, 2024 14:30

use isKnownGT to compare LLT and TargetRegister

89c8fd2

getRegClassForTypeOnBank for vector register bank

73910bb

vector instr sel for add and sub

f3644c9

getInstrMapping for vector add and sub; regbankselect test cases for …

e7a43e6

…vector add and sub

drop curly braces around single lines

585d8db

correct vectorized G_ADD, G_SUB instruction selection test cases

f20faf0

partial mapping VRB64 is for LMUL=1, MF2, MF4, and MF8

177d866

clang format

12ab39a

delete todo

944dd0d

put partial mappings to single lines

fda9565

regbankselect test cases for vectorized G_ADD and G_SUB

766a7aa

clang format off to put PartMappings in single lines

5d60e8d

jiahanxie353 force-pushed the instr-sel branch from 66db262 to 5d60e8d Compare January 31, 2024 19:35

michaelmaitland approved these changes Feb 1, 2024

View reviewed changes

jiahanxie353 merged commit 10c2d5f into llvm:main Feb 1, 2024
4 checks passed

jiahanxie353 deleted the instr-sel branch February 1, 2024 20:06

	// Maximum 3 VR LMUL=1 operands.
	// Maximum 3 VR LMUL={1, MF2, MF4, MF8} operands.

		@@ -0,0 +1,556 @@
		# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
		# RUN: llc -mtriple=riscv32 -mattr=+v -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - \

		@@ -785,6 +785,21 @@ const TargetRegisterClass *RISCVInstructionSelector::getRegClassForTypeOnBank(
		}

		// TODO: Non-GPR register classes.

[RISCV][GISel] RegBank select and instruction select for vector G_ADD, G_SUB #74114

[RISCV][GISel] RegBank select and instruction select for vector G_ADD, G_SUB #74114

Conversation

jiahanxie353 commented Dec 1, 2023 • edited

github-actions bot commented Dec 1, 2023 • edited

jiahanxie353 commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Dec 1, 2023 • edited

topperc commented Dec 1, 2023

michaelmaitland commented Dec 1, 2023 • edited

jiahanxie353 commented Dec 1, 2023

michaelmaitland commented Dec 1, 2023

jiahanxie353 commented Dec 1, 2023

jiahanxie353 commented Dec 5, 2023

michaelmaitland commented Dec 5, 2023

jiahanxie353 commented Dec 5, 2023

michaelmaitland commented Dec 5, 2023 • edited

jiahanxie353 commented Dec 6, 2023

michaelmaitland commented Dec 6, 2023

jiahanxie353 commented Dec 8, 2023

michaelmaitland commented Dec 8, 2023

michaelmaitland commented Dec 8, 2023

topperc commented Dec 8, 2023

jiahanxie353 commented Dec 11, 2023

michaelmaitland commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmaitland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmaitland commented Jan 16, 2024

michaelmaitland left a comment

Choose a reason for hiding this comment

jiahanxie353 commented Dec 1, 2023 •

edited

github-actions bot commented Dec 1, 2023 •

edited

llvmbot commented Dec 1, 2023 •

edited

michaelmaitland commented Dec 1, 2023 •

edited

michaelmaitland commented Dec 5, 2023 •

edited