[RISC-V][GISEL] Select G_BITCAST for scalable vectors #101486

jiahanxie353 · 2024-08-01T13:37:41Z

Just got started to work on instruction selection after landing the regbankselect pass for scalable vector loads/stores.

Seems like selectImpl can capture loads/stores, but is not able to work for G_BITCAST out of the box.

Would a bitcast essentially being the same semantics as a copy? I was also looking at how AArch64 did it, seems like they are also just doing a copy? I'm not entirely sure.

Can I get some pointers? @topperc @michaelmaitland Thanks!

michaelmaitland · 2024-08-01T13:59:35Z

According to RISCVDAGToADAGISel::Select, we should drop bitcasts between vectors if both are fixed or both are scalable.

topperc · 2024-08-01T14:10:39Z

According to RISCVDAGToADAGISel::Select, we should drop bitcasts between vectors if both are fixed or both are scalable.

We should emit a COPY for GISel. Otherwise we have to change registers on other instructions to emit nothing.

jiahanxie353 · 2024-08-05T16:58:58Z

Is there a corresponding PseudosV* during instruction selection for load/storing a scalable vector of pointers?

michaelmaitland · 2024-08-05T17:52:28Z

Is there a corresponding PseudosV* during instruction selection for load/storing a scalable vector of pointers?

It may be useful to look at RISCVDAGToDAGISel::Select for ISD::LOAD to see what is done there.

Also in RISCVInstrInfoVSDPatterns.td there are some patterns for load/store.

https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#sec-vector-memory might give you a better idea of what kind of vector loads we have. The goal is to pick the best kind of RVV load for a LLVM load instruciton.

Since pointers use GPR, I think that they use the normal patterns.

michaelmaitland · 2024-09-10T21:04:59Z

Need tests?

llvmbot · 2024-09-10T21:05:45Z

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

Author: Jiahan Xie (jiahanxie353)

Changes

Just got started to work on instruction selection after landing the regbankselect pass for scalable vector loads/stores.

Seems like selectImpl can capture loads/stores, but is not able to work for G_BITCAST out of the box.

Would a bitcast essentially being the same semantics as a copy? I was also looking at how AArch64 did it, seems like they are also just doing a copy? I'm not entirely sure.

Can I get some pointers? @topperc @michaelmaitland Thanks!

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

1 Files Affected:

(modified) llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp (+1)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index fdb1ebace00107..2243a22a167cab 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -559,6 +559,7 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
   case TargetOpcode::G_INTTOPTR:
   case TargetOpcode::G_TRUNC:
   case TargetOpcode::G_FREEZE:
+  case TargetOpcode::G_BITCAST:
     return selectCopy(MI, MRI);
   case TargetOpcode::G_CONSTANT: {
     Register DstReg = MI.getOperand(0).getReg();

jiahanxie353 · 2024-09-16T20:21:07Z

Currently, we need to support load/store of a vector of pointers. For GISEL, we need to define some new patterns inRISCVInstrInfoVPseudos.td.

If I were to do it in a brute-force way, should we get something like the following (largely copied from AllIntegerVectors):

    def nxv1i32p0 = VTScalableVec<1, i32, 0>;
    def nxv2i32p0 = VTScalableVec<2, i32, 0>;
    def nxv4i32p0 = VTScalableVec<4, i32, 0>;
    def nxv8i32p0 = VTScalableVec<8, i32, 0>;
    def nxv1i64p0 = VTScalableVec<1, i64, 0>;
    def nxv2i64p0 = VTScalableVec<2, i64, 0>;
    def nxv4i64p0 = VTScalableVec<4, i64, 0>;
    def nxv8i64p0 = VTScalableVec<8, i64, 0>;

    defvar vptr32mf2_t = nxv1i32p0;
    defvar vptr32m1_t = nxv2i32p0;
    defvar vptr32m2_t = nxv4i32p0;
    defvar vptr32m4_t = nxv8i32p0;
    defvar vptr64m1_t = nxv1i64p0;
    defvar vptr64m2_t = nxv2i64p0;
    defvar vptr64m4_t = nxv4i64p0;
    defvar vptr64m8_t = nxv8i64p0;

    defset list<VTypeInfo> PtrVectors = {
      def VPTR32MF2: VTypeInfo<vptr32mf2_t, vbool64_t, 32, V_MF2>;
      def VPTR32M1: VTypeInfo<vptr32m1_t, vbool32_t, 32, V_M1>;
      def VPTR32M2: GroupVTypeInfo<vptr32m2_t, vptr32m1_t, vbool16_t, 32, V_M2>;
      def VPTR32M4: GroupVTypeInfo<vptr32m4_t, vptr32m1_t, vbool8_t, 32, V_M4>;

      def VPTR64M1: VTypeInfo<vptr64m1_t, vbool64_t, 64, V_M1>;
      def VPTR64M2: GroupVTypeInfo<vptr64m2_t, vptr64m1_t, vbool32_t, 64, V_M2>;
      def VPTR64M4: GroupVTypeInfo<vptr64m4_t, vptr64m1_t, vbool16_t, 64, V_M4>;
      def VPTR64M8: GroupVTypeInfo<vptr64m8_t, vptr64m1_t, vbool8_t, 64, V_M8>;
    }
  }

did I write the patterns correctly?

And then in RISCVGISEL.td:

let Predicates = [IsRV32] in {
foreach vti = PtrVectors in
  def VPatUSLoadStoreSDNode<vti.Vector, vti.Log2SEW, vti.LMul, vti.AVL, vti.RegClass>;
}

let Predicates = [IsRV64] in ...

Even if I wrote the patterns correctly, the issue is that we should really have XLen in place of explicit i32/i64:

    def p0XLen : PtrValueType<XLenVT, 0>;
    def  nxv1p0 = VTScalableVec<1, p0XLen, 0>;
    def  nxv2p0 = VTScalableVec<2, p0XLen, 0>;
    def  nxv4p0 = VTScalableVec<4, p0XLen, 0>;
    def  nxv8p0 = VTScalableVec<8, p0XLen, 0>;

And how should we use XLen uniformly instead of brute-force 32-bit and 64-bit machine separately?

topperc · 2024-09-16T20:29:11Z

Currently, we need to support load/store of a vector of pointers. For GISEL, we need to define some new patterns inRISCVInstrInfoVPseudos.td.

If I were to do it in a brute-force way, should we get something like the following (largely copied from AllIntegerVectors):

    def nxv1i32p0 = VTScalableVec<1, i32, 0>;
    def nxv2i32p0 = VTScalableVec<2, i32, 0>;
    def nxv4i32p0 = VTScalableVec<4, i32, 0>;
    def nxv8i32p0 = VTScalableVec<8, i32, 0>;
    def nxv1i64p0 = VTScalableVec<1, i64, 0>;
    def nxv2i64p0 = VTScalableVec<2, i64, 0>;
    def nxv4i64p0 = VTScalableVec<4, i64, 0>;
    def nxv8i64p0 = VTScalableVec<8, i64, 0>;

    defvar vptr32mf2_t = nxv1i32p0;
    defvar vptr32m1_t = nxv2i32p0;
    defvar vptr32m2_t = nxv4i32p0;
    defvar vptr32m4_t = nxv8i32p0;
    defvar vptr64m1_t = nxv1i64p0;
    defvar vptr64m2_t = nxv2i64p0;
    defvar vptr64m4_t = nxv4i64p0;
    defvar vptr64m8_t = nxv8i64p0;

    defset list<VTypeInfo> PtrVectors = {
      def VPTR32MF2: VTypeInfo<vptr32mf2_t, vbool64_t, 32, V_MF2>;
      def VPTR32M1: VTypeInfo<vptr32m1_t, vbool32_t, 32, V_M1>;
      def VPTR32M2: GroupVTypeInfo<vptr32m2_t, vptr32m1_t, vbool16_t, 32, V_M2>;
      def VPTR32M4: GroupVTypeInfo<vptr32m4_t, vptr32m1_t, vbool8_t, 32, V_M4>;

      def VPTR64M1: VTypeInfo<vptr64m1_t, vbool64_t, 64, V_M1>;
      def VPTR64M2: GroupVTypeInfo<vptr64m2_t, vptr64m1_t, vbool32_t, 64, V_M2>;
      def VPTR64M4: GroupVTypeInfo<vptr64m4_t, vptr64m1_t, vbool16_t, 64, V_M4>;
      def VPTR64M8: GroupVTypeInfo<vptr64m8_t, vptr64m1_t, vbool8_t, 64, V_M8>;
    }
  }

did I write the patterns correctly?

And then in RISCVGISEL.td:

let Predicates = [IsRV32] in {
foreach vti = PtrVectors in
  def VPatUSLoadStoreSDNode<vti.Vector, vti.Log2SEW, vti.LMul, vti.AVL, vti.RegClass>;
}

let Predicates = [IsRV64] in ...

Even if I wrote the patterns correctly, the issue is that we should really have XLen in place of explicit i32/i64:

    def p0XLen : PtrValueType<XLenVT, 0>;
    def  nxv1p0 = VTScalableVec<1, p0XLen, 0>;
    def  nxv2p0 = VTScalableVec<2, p0XLen, 0>;
    def  nxv4p0 = VTScalableVec<4, p0XLen, 0>;
    def  nxv8p0 = VTScalableVec<8, p0XLen, 0>;

And how should we use XLen uniformly instead of brute-force 32-bit and 64-bit machine separately?

AArch64 handles scalar load/stores of pointers in AArch64InstructionSelector::preISelLower by forcing the type to integer. Can we do something similar?

jiahanxie353 · 2024-09-23T20:53:19Z

Seems like selectImpl can capture loads/stores,

I was wrong, selectImpl only supports rv32 for loading this example but not rv64.. it seems pretty weird to me since the table tells us it should work with rv64 also, right?

github-actions · 2024-09-23T20:53:26Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 81c3499531c3fe03827bd8bc890e3a16db9e4c3c 057c06a585b528be37b735feac47e73e8cda1389 --extensions cpp -- llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index 78fffa5047..e484d2af15 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -814,7 +814,9 @@ void RISCVInstructionSelector::preISelLower(MachineInstr &MI,
         "through preISelLower")
       break;
     const LLT sXLen = LLT::scalar(STI.getXLen());
-    auto Copy = MIB.buildCopy(LLT::scalable_vector(SrcTy.getElementCount().getKnownMinValue(), sXLen), SrcOp);
+    auto Copy = MIB.buildCopy(
+        LLT::scalable_vector(SrcTy.getElementCount().getKnownMinValue(), sXLen),
+        SrcOp);
     Register NewSrc = Copy.getReg(0);
     SrcOp.setReg(NewSrc);
   }

michaelmaitland · 2024-09-23T21:07:58Z

Could you update PR description and title please?

michaelmaitland

I am looking into why this is failing for rv64.

michaelmaitland · 2024-09-23T21:08:28Z

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

@@ -559,6 +559,7 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
  case TargetOpcode::G_INTTOPTR:
  case TargetOpcode::G_TRUNC:
  case TargetOpcode::G_FREEZE:
+  case TargetOpcode::G_BITCAST:


We're going to need G_BITCAST instruction-select tests due to this line

michaelmaitland · 2024-09-23T21:08:54Z

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

+# 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 - | FileCheck -check-prefix=RV32I %s
+# RUN: llc -mtriple=riscv64 -mattr=+v -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - | FileCheck -check-prefix=RV64I %s
+--- |


Are we able to drop the LLVM IR part of this file (i.e. what is between this ---| and ...)? You will probably need to rename bb.1 (%ir-block.0): to bb.1: in the corresponding MIR functions below since we removed the LLVM IR.

michaelmaitland · 2024-09-23T21:18:59Z

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

+  case TargetOpcode::G_LOAD: {
+    Register DstReg = MI.getOperand(0).getReg();
+    const LLT DstTy = MRI.getType(DstReg);
+    if (!(DstTy.isVector() && DstTy.getElementType().isPointer()))


Simplify to !DestTy.isVector() || !DstTy.isPointerVector().

would !DstTy.isPointerVector() suffice since that also checks if DestTy is a Vector or not?

Even better!

michaelmaitland · 2024-09-23T21:19:20Z

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

+  case TargetOpcode::G_STORE: {
+    MachineOperand &SrcOp = MI.getOperand(0);
+    const LLT SrcTy = MRI.getType(SrcOp.getReg());
+    if (!(SrcTy.isVector() && SrcTy.getElementType().isPointer()))


Simplify to !SrcTy.isVector() || !SrcTy.isPointerVector().

michaelmaitland · 2024-09-23T21:20:17Z

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

+    if (!(SrcTy.isVector() && SrcTy.getElementType().isPointer()))
+        break;
+    const LLT sXLen = LLT::scalar(STI.getXLen());
+    auto Copy = MIB.buildCopy(LLT::scalable_vector(SrcTy.getElementCount().getKnownMinValue(), sXLen), SrcOp);


Why do we do a copy instead of changing the store's vector pointer element type to vector of sXLen type?

the pointer has p0 type. we need to change the type of the data to store, but that type is owned by the instruction that produces the data. so you can't change it. if you were to change it that instruction would fail instruction selection when we visit it.

michaelmaitland · 2024-09-23T21:39:39Z

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

@@ -785,6 +786,26 @@ void RISCVInstructionSelector::preISelLower(MachineInstr &MI,
    replacePtrWithInt(MI.getOperand(1), MIB, MRI);
    MI.setDesc(TII.get(TargetOpcode::G_AND));
    MRI.setType(DstReg, sXLen);
+    break;
+  }
+  case TargetOpcode::G_LOAD: {


We have a helper function called replacePtrWithInt. Maybe you can expand that to support vectors instead of doing it here in the code manually?

topperc · 2024-09-23T22:00:05Z

I am looking into why this is failing for rv64.

It's failing because only the RV32 pattern exists in lib/Target/RISCV/RISCVGenGlobalISel.inc

michaelmaitland · 2024-09-23T22:05:26Z

It looks like it is failing 64543: GIM_CheckFeatures(ExpectedBitsetID=47). Based on what I can tell, that occurs when we don't have the correct subtarget features. I thought you have specified the correct subtarget features though (via mtriple and mattr). I am continuing to look into it.

EDIT: Just saw what @topperc posted, I guess it has to do with that.

topperc · 2024-09-23T23:05:59Z

I am looking into why this is failing for rv64.

It's failing because only the RV32 pattern exists in lib/Target/RISCV/RISCVGenGlobalISel.inc

Should be fixed after 9d3a576

jiahanxie353 · 2024-09-24T13:10:28Z

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

+         "Operand must be a pointer of a vector of pointers");
+  assert(PtrTy.isScalableVector() &&
+         "Currently only working for scalable vector of pointers now");
+  MRI.setType(PtrReg, LLT::scalable_vector(


We have a helper function called replacePtrWithInt. Maybe you can expand that to support vectors instead of doing it here in the code manually?

Not sure if makes sense to inplace update and return true, instead of building a new instruction like G_PTRTOINT. I didn't do it because there is no generic opcode for it and we have to go through extract_elment and apply each element with G_PTRTOINT which feels very cumbersome.

It looks like AArch64 changes the type without building a PtrToInt. They say it works because all users have been selected already (bottom up selection) so the type does not matter for them. I think we can do the same thing, which is what you have done here.

I wonder if we really needed to build the PtrToInt above for the same reason. I can take a look at that in a follow up patch.

If the vector and non-vector code do something wildly different they should not be in the same function.

michaelmaitland · 2024-09-24T15:07:35Z

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

+        "through preISelLower")
+      break;
+    const LLT sXLen = LLT::scalar(STI.getXLen());
+    auto Copy = MIB.buildCopy(LLT::scalable_vector(SrcTy.getElementCount().getKnownMinValue(), sXLen), SrcOp);


nit: clang format

michaelmaitland self-requested a review September 10, 2024 21:04

michaelmaitland added the backend:RISC-V label Sep 10, 2024

jiahanxie353 added 3 commits September 22, 2024 22:27

bitcast as copy?

cea2c3e

add instr select tests for load and store

7d9d4f2

pre instr select lower to force load/store of pointers to xlen integers

907dbb3

jiahanxie353 force-pushed the instr-sel branch from 5fea25e to 907dbb3 Compare September 23, 2024 20:49

michaelmaitland requested a review from topperc September 23, 2024 21:06

michaelmaitland reviewed Sep 23, 2024

View reviewed changes

jiahanxie353 added 2 commits September 24, 2024 08:43

update tests to remove llvm ir and update test checks

96e23b4

use replacePtrWithInt for G_LOAD

057c06a

jiahanxie353 commented Sep 24, 2024

View reviewed changes

michaelmaitland reviewed Sep 24, 2024

View reviewed changes

[RISC-V][GISEL] Select G_BITCAST for scalable vectors #101486

Are you sure you want to change the base?

[RISC-V][GISEL] Select G_BITCAST for scalable vectors #101486

Uh oh!

Conversation

jiahanxie353 commented Aug 1, 2024

Uh oh!

michaelmaitland commented Aug 1, 2024

Uh oh!

topperc commented Aug 1, 2024

Uh oh!

jiahanxie353 commented Aug 5, 2024

Uh oh!

michaelmaitland commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelmaitland commented Sep 10, 2024

Uh oh!

llvmbot commented Sep 10, 2024

Uh oh!

jiahanxie353 commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Sep 16, 2024

Uh oh!

jiahanxie353 commented Sep 23, 2024

Uh oh!

github-actions bot commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelmaitland commented Sep 23, 2024

Uh oh!

michaelmaitland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelmaitland commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelmaitland commented Aug 5, 2024 •

edited

Loading

jiahanxie353 commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 23, 2024 •

edited

Loading

topperc Sep 23, 2024 •

edited

Loading

topperc commented Sep 23, 2024 •

edited

Loading

michaelmaitland commented Sep 23, 2024 •

edited

Loading