Skip to content

[RISCV] Don't move source if passthru already dominates in vmv.v.v peephole #105792

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

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Aug 23, 2024

Currently we move the source down to where vmv.v.v to make sure that the new passthru dominates, but we do this even if it already does.

This adds a simple local dominance check (taken from X86FastPreTileConfig.cpp) and avoids doing the move if it can.

It also modifies the move to only move it to just past the passthru definition, and not all the way down to the vmv.v.v.

This allows folding to succeed in some edge cases, which prevents regressions in an upcoming patch.

…ephole

Currently we move the source down to where vmv.v.v to make sure that the new passthru dominates, but we do this even if it already does.

This adds a simple local dominance check (taken from X86FastPreTileConfig.cpp) and avoids doing the move if it can.

It also modifies the move to only move it to just past the passthru definition, and not all the way down to the vmv.v.v.

This allows folding to succeed in some edge cases, which prevents regressions in an upcoming patch.
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2024

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

Author: Luke Lau (lukel97)

Changes

Currently we move the source down to where vmv.v.v to make sure that the new passthru dominates, but we do this even if it already does.

This adds a simple local dominance check (taken from X86FastPreTileConfig.cpp) and avoids doing the move if it can.

It also modifies the move to only move it to just past the passthru definition, and not all the way down to the vmv.v.v.

This allows folding to succeed in some edge cases, which prevents regressions in an upcoming patch.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp (+25-6)
  • (added) llvm/test/CodeGen/RISCV/rvv/vmv.v.v-peephole.mir (+20)
diff --git a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
index 9772782ad3d6db..822ab492c710b4 100644
--- a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
@@ -437,6 +437,22 @@ static bool isSafeToMove(const MachineInstr &From, const MachineInstr &To) {
   return From.isSafeToMove(SawStore);
 }
 
+/// Given A and B are in the same MBB, returns true if A comes before B.
+static bool dominates(MachineBasicBlock::const_iterator A,
+                      MachineBasicBlock::const_iterator B) {
+  assert(A->getParent() == B->getParent());
+  const MachineBasicBlock *MBB = A->getParent();
+  auto MBBEnd = MBB->end();
+  if (B == MBBEnd)
+    return true;
+
+  MachineBasicBlock::const_iterator I = MBB->begin();
+  for (; &*I != A && &*I != B; ++I)
+    ;
+
+  return &*I == A;
+}
+
 /// If a PseudoVMV_V_V is the only user of its input, fold its passthru and VL
 /// into it.
 ///
@@ -481,12 +497,15 @@ bool RISCVVectorPeephole::foldVMV_V_V(MachineInstr &MI) {
   if (!isVLKnownLE(SrcVL, MI.getOperand(3)))
     return false;
 
-  // If Src ends up using MI's passthru/VL, move it so it can access it.
-  // TODO: We don't need to do this if they already dominate Src.
-  if (!SrcPassthru.isIdenticalTo(Passthru)) {
-    if (!isSafeToMove(*Src, MI))
-      return false;
-    Src->moveBefore(&MI);
+  // If the new passthru doesn't dominate Src, try to move Src so it does.
+  if (Passthru.getReg() != RISCV::NoRegister) {
+    MachineInstr *PassthruDef = MRI->getVRegDef(Passthru.getReg());
+    if (PassthruDef->getParent() == Src->getParent() &&
+        !dominates(PassthruDef, Src)) {
+      if (!isSafeToMove(*Src, *PassthruDef->getNextNode()))
+        return false;
+      Src->moveBefore(PassthruDef->getNextNode());
+    }
   }
 
   if (SrcPassthru.getReg() != Passthru.getReg()) {
diff --git a/llvm/test/CodeGen/RISCV/rvv/vmv.v.v-peephole.mir b/llvm/test/CodeGen/RISCV/rvv/vmv.v.v-peephole.mir
new file mode 100644
index 00000000000000..b2526c6df6939e
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/vmv.v.v-peephole.mir
@@ -0,0 +1,20 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3
+# RUN: llc %s -o - -mtriple=riscv64 -mattr=+v -run-pass=riscv-vector-peephole \
+# RUN:     -verify-machineinstrs | FileCheck %s
+
+---
+name: move_src
+body: |
+  bb.0:
+    liveins: $v8
+    ; CHECK-LABEL: name: move_src
+    ; CHECK: liveins: $v8
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %passthru:vr = COPY $v8
+    ; CHECK-NEXT: %x:vr = PseudoVADD_VV_M1 %passthru, $noreg, $noreg, 4, 5 /* e32 */, 0 /* tu, mu */
+    ; CHECK-NEXT: %y:gpr = ADDI $x0, 1
+    %x:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, 4, 5 /* e32 */, 0 /* tu, mu */
+    %passthru:vr = COPY $v8
+    %y:gpr = ADDI $x0, 1
+    %z:vr = PseudoVMV_V_V_M1 %passthru, %x, 4, 5 /* e32 */, 0 /* tu, mu */
+...

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

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

LGTM

@lukel97 lukel97 merged commit be5ecc3 into llvm:main Aug 24, 2024
8 of 10 checks passed
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Sep 30, 2024
…3baa081ca

Local branch amd-gfx fee3baa Merged main:99b85cae628c1cc5641944290712cd84ccf1f6c8 into amd-gfx:f09ee8740921
Remote branch main be5ecc3 [RISCV] Dont move source if passthru already dominates in vmv.v.v peephole (llvm#105792)
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.

3 participants