Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV][GISEL] instruction-select vmclr #110782

Merged
merged 10 commits into from
Oct 4, 2024

Conversation

michaelmaitland
Copy link
Contributor

@michaelmaitland michaelmaitland commented Oct 2, 2024

This is stacked on #110778. This PR adds and tests renderVLOp too, as that is needed from vmclr.

@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-llvm-globalisel

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

Author: Michael Maitland (michaelmaitland)

Changes

This is stacked on #110778


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

4 Files Affected:

  • (modified) llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp (+22)
  • (modified) llvm/lib/Target/RISCV/RISCVGISel.td (+5)
  • (added) llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv32.mir (+124)
  • (added) llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv64.mir (+124)
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index 92d00c26bd219c..cb28f40d64b176 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -100,6 +100,8 @@ class RISCVInstructionSelector : public InstructionSelector {
     return selectSHXADD_UWOp(Root, ShAmt);
   }
 
+  ComplexRendererFns selectVLOp(MachineOperand &Root) const;
+
   // Custom renderers for tablegen
   void renderNegImm(MachineInstrBuilder &MIB, const MachineInstr &MI,
                     int OpIdx) const;
@@ -379,6 +381,26 @@ RISCVInstructionSelector::selectSHXADD_UWOp(MachineOperand &Root,
   return std::nullopt;
 }
 
+InstructionSelector::ComplexRendererFns
+RISCVInstructionSelector::selectVLOp(MachineOperand &Root) const {
+  MachineRegisterInfo &MRI =
+      Root.getParent()->getParent()->getParent()->getRegInfo();
+  assert(Root.isReg() && "Expected operand to be a Register");
+  MachineInstr *RootDef = MRI.getVRegDef(Root.getReg());
+
+  if (RootDef->getOpcode() == TargetOpcode::G_CONSTANT &&
+      RootDef->getOperand(1).getCImm()->getSExtValue() == RISCV::VLMaxSentinel)
+    // If the operand is a G_CONSTANT with value VLMaxSentinel, convert it
+    // to an immediate with value VLMaxSentinel. This is recognized specially by
+    // the vsetvli insertion pass.
+    return {
+        {[=](MachineInstrBuilder &MIB) { MIB.addImm(RISCV::VLMaxSentinel); }}};
+
+  // FIXME: Implement non-VLMAX case. ISEL will fail gracefully by returning
+  // like this for now.
+  return std::nullopt;
+}
+
 InstructionSelector::ComplexRendererFns
 RISCVInstructionSelector::selectAddrRegImm(MachineOperand &Root) const {
   MachineFunction &MF = *Root.getParent()->getParent()->getParent();
diff --git a/llvm/lib/Target/RISCV/RISCVGISel.td b/llvm/lib/Target/RISCV/RISCVGISel.td
index 319611111cf470..a7d46e6edd39f1 100644
--- a/llvm/lib/Target/RISCV/RISCVGISel.td
+++ b/llvm/lib/Target/RISCV/RISCVGISel.td
@@ -50,6 +50,11 @@ def GIAddrRegImm :
   GIComplexOperandMatcher<s32, "selectAddrRegImm">,
   GIComplexPatternEquiv<AddrRegImm>;
 
+def GIVLOpS32 : GIComplexOperandMatcher<s32, "selectVLOp">,
+                GIComplexPatternEquiv<VLOp>;
+def GIVLOpS64 : GIComplexOperandMatcher<s64, "selectVLOp">,
+                GIComplexPatternEquiv<VLOp>;
+
 // Convert from i32 immediate to i64 target immediate to make SelectionDAG type
 // checking happy so we can use ADDIW which expects an XLen immediate.
 def as_i64imm : SDNodeXForm<imm, [{
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv32.mir b/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv32.mir
new file mode 100644
index 00000000000000..1ef1312cc17c0e
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv32.mir
@@ -0,0 +1,124 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv32 -mattr=+v,+m -run-pass=instruction-select \
+# RUN:   -simplify-mir -verify-machineinstrs %s -o - | FileCheck %s
+
+---
+name:            splat_zero_nxv1i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv1i1
+    ; CHECK: [[PseudoVMCLR_M_B1_:%[0-9]+]]:vr = PseudoVMCLR_M_B1 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B1_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 1 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 1 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv2i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv2i1
+    ; CHECK: [[PseudoVMCLR_M_B2_:%[0-9]+]]:vr = PseudoVMCLR_M_B2 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B2_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 2 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 2 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv4i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv4i1
+    ; CHECK: [[PseudoVMCLR_M_B4_:%[0-9]+]]:vr = PseudoVMCLR_M_B4 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B4_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 4 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 4 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv8i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv8i1
+    ; CHECK: [[PseudoVMCLR_M_B8_:%[0-9]+]]:vr = PseudoVMCLR_M_B8 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B8_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 8 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 8 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv16i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv16i1
+    ; CHECK: [[PseudoVMCLR_M_B16_:%[0-9]+]]:vr = PseudoVMCLR_M_B16 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B16_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 16 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 16 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv32i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv32i1
+    ; CHECK: [[PseudoVMCLR_M_B32_:%[0-9]+]]:vr = PseudoVMCLR_M_B32 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B32_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 32 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 32 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv64i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv64i1
+    ; CHECK: [[PseudoVMCLR_M_B64_:%[0-9]+]]:vr = PseudoVMCLR_M_B64 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B64_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s32) = G_CONSTANT i32 -1
+    %1:vrb(<vscale x 64 x s1>) = G_VMCLR_VL %0(s32)
+    $v0 = COPY %1(<vscale x 64 x s1>)
+    PseudoRET implicit $v0
+
+...
+
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv64.mir b/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv64.mir
new file mode 100644
index 00000000000000..b7541cd4e96fb4
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/rvv/vmclr-rv64.mir
@@ -0,0 +1,124 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=riscv64 -mattr=+v,+m -run-pass=instruction-select \
+# RUN:   -simplify-mir -verify-machineinstrs %s -o - | FileCheck %s
+
+---
+name:            splat_zero_nxv1i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv1i1
+    ; CHECK: [[PseudoVMCLR_M_B1_:%[0-9]+]]:vr = PseudoVMCLR_M_B1 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B1_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 1 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 1 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv2i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv2i1
+    ; CHECK: [[PseudoVMCLR_M_B2_:%[0-9]+]]:vr = PseudoVMCLR_M_B2 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B2_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 2 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 2 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv4i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv4i1
+    ; CHECK: [[PseudoVMCLR_M_B4_:%[0-9]+]]:vr = PseudoVMCLR_M_B4 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B4_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 4 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 4 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv8i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv8i1
+    ; CHECK: [[PseudoVMCLR_M_B8_:%[0-9]+]]:vr = PseudoVMCLR_M_B8 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B8_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 8 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 8 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv16i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv16i1
+    ; CHECK: [[PseudoVMCLR_M_B16_:%[0-9]+]]:vr = PseudoVMCLR_M_B16 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B16_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 16 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 16 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv32i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv32i1
+    ; CHECK: [[PseudoVMCLR_M_B32_:%[0-9]+]]:vr = PseudoVMCLR_M_B32 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B32_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 32 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 32 x s1>)
+    PseudoRET implicit $v0
+
+...
+---
+name:            splat_zero_nxv64i1
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: splat_zero_nxv64i1
+    ; CHECK: [[PseudoVMCLR_M_B64_:%[0-9]+]]:vr = PseudoVMCLR_M_B64 -1, 0 /* e8 */
+    ; CHECK-NEXT: $v0 = COPY [[PseudoVMCLR_M_B64_]]
+    ; CHECK-NEXT: PseudoRET implicit $v0
+    %0:gprb(s64) = G_CONSTANT i64 -1
+    %1:vrb(<vscale x 64 x s1>) = G_VMCLR_VL %0(s64)
+    $v0 = COPY %1(<vscale x 64 x s1>)
+    PseudoRET implicit $v0
+
+...
+

@@ -50,6 +50,11 @@ def GIAddrRegImm :
GIComplexOperandMatcher<s32, "selectAddrRegImm">,
GIComplexPatternEquiv<AddrRegImm>;

def GIVLOpS32 : GIComplexOperandMatcher<s32, "selectVLOp">,
GIComplexPatternEquiv<VLOp>;
def GIVLOpS64 : GIComplexOperandMatcher<s64, "selectVLOp">,
Copy link
Collaborator

@topperc topperc Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this works. The pattern refers to VLOp. How does tablegen know which of these GIComplexOperandMatcher to pick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that it'll try to match s32, and if it succeeds it will do the RISCVInstructionSelector::selectVLOp in place of RISCVDAGToDAGISel::selectVLOp, which is associated with def VLOp : ComplexPattern<XLenVT, 1, "selectVLOp">;. If it fails, it will repeat for i64. Only one will be true at a time because the operand cannot be both s32 and s64.

But maybe someone else can weigh in here, because I am not too familiar with how this works.

Copy link
Collaborator

@topperc topperc Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is how it works in this tablegen code which can only remember one of the values.

  for (const Record *Equiv :                                                     
       RK.getAllDerivedDefinitions("GIComplexPatternEquiv")) {                   
    const Record *SelDAGEquiv = Equiv->getValueAsDef("SelDAGEquivalent");        
    if (!SelDAGEquiv)                                                            
      continue;                                                                  
    ComplexPatternEquivs[SelDAGEquiv] = Equiv;                                   
  }

It's a map from the SelectionDAG node to the GISel node. The second one will overwrite the first one in the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests to show that it is working as expected for rv32 and rv64.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you delete one of them? Does it still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works if I delete one of them. As we discussed offline, maybe the whole type thing isn't actually implemented correctly. I've updated the patch to call this out.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure it works because there is an explicit cast around the VLOp part in this pattern def VLOpFrag : PatFrag<(ops), (XLenVT (VLOp (XLenVT AVL:$vl)))>

@topperc
Copy link
Collaborator

topperc commented Oct 2, 2024

Please add an end to end IR test.

@michaelmaitland
Copy link
Contributor Author

Please add an end to end IR test.

Will do in morning. Off to bed now.

@@ -379,6 +381,30 @@ RISCVInstructionSelector::selectSHXADD_UWOp(MachineOperand &Root,
return std::nullopt;
}

InstructionSelector::ComplexRendererFns
RISCVInstructionSelector::renderVLOp(MachineOperand &Root) const {
MachineRegisterInfo &MRI =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a member of RISCVInstructionSelector if it's not already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. Is it okay for me to do this as a follow up? There are many other functions in this file that are getting MRI like this that need refactoring too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@michaelmaitland
Copy link
Contributor Author

Just pushed a rebase.

michaelmaitland added a commit to michaelmaitland/llvm-project that referenced this pull request Oct 2, 2024
It was requested in llvm#110782 (comment)
that MRI be made a member of RISCVInstructionSelector.

RISCVInstructionSelector is created in the RISCVSubtarget, independent of
MachineFunction. So it cannot be passed by reference during construction of
RISCVInstructionSelector.

The MachineRegisterInfo object belongs to each MachineFunction, so we will
set it as we enter `select`, which is the only public function to
RISCVInstructionSelector. We don't need to worry about clearing it before
returning from `select`, since there is no other entry point.

I'm not sure this is any better than what we have today. Not sure whether
we should take this change or not. If in the future we have other public
functions of RISCVInstructionSelector, we will need to be more careful about
checking that MRI is set and clearing it where appropriate.
michaelmaitland added a commit that referenced this pull request Oct 4, 2024
…10926)

It was requested in
#110782 (comment)
that MRI be made a member of RISCVInstructionSelector.

RISCVInstructionSelector is created in the RISCVSubtarget, independent
of MachineFunction. So it cannot be passed by reference during
construction of RISCVInstructionSelector.

The MachineRegisterInfo object belongs to each MachineFunction, so
set it in setupMF.
@michaelmaitland michaelmaitland merged commit f873fc3 into llvm:main Oct 4, 2024
9 checks passed
@michaelmaitland michaelmaitland deleted the isel-vmclr branch October 4, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants