[RISCV][MCP] Remove redundant move from tail duplication #89865

BeMg · 2024-04-24T03:13:59Z

Tail duplication will generate the redundant move before return. It is because the MachineCopyPropogation can't recognize COPY after post-RA pseudoExpand.

This patch make MachineCopyPropogation recognize %0 = ADDI %1, 0 as COPY

stack on #89864

llvmbot · 2024-04-24T03:14:29Z

@llvm/pr-subscribers-llvm-regalloc

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

Author: Piyou Chen (BeMg)

Changes

Tail duplication will generate the redundant move before return. It is because the MachineCopyPropogation can't recognize COPY after post-RA pseudoExpand.

This patch

Keep renamable after post-RA pseudoExpand
Let MachineCopyPropogation recognize %0 = ADDI %1, 0 as COPY

stack on #89864

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

3 Files Affected:

(modified) llvm/lib/CodeGen/MachineCopyPropagation.cpp (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+5-3)
(added) llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll (+50)

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 8dc6781fcb018f..5aaf6b31dcc542 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -1053,7 +1053,7 @@ void MachineCopyPropagation::BackwardCopyPropagateBlock(
     // Ignore non-trivial COPYs.
     std::optional<DestSourcePair> CopyOperands =
         isCopyInstr(MI, *TII, UseCopyInstr);
-    if (CopyOperands && MI.getNumOperands() == 2) {
+    if (CopyOperands) {
       Register DefReg = CopyOperands->Destination->getReg();
       Register SrcReg = CopyOperands->Source->getReg();
 
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 70ac1f8a592e02..c05bf8b0f68e08 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -424,9 +424,11 @@ void RISCVInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
   const TargetRegisterInfo *TRI = STI.getRegisterInfo();
 
   if (RISCV::GPRRegClass.contains(DstReg, SrcReg)) {
-    BuildMI(MBB, MBBI, DL, get(RISCV::ADDI), DstReg)
-        .addReg(SrcReg, getKillRegState(KillSrc))
-        .addImm(0);
+    auto MI = BuildMI(MBB, MBBI, DL, get(RISCV::ADDI), DstReg)
+                  .addReg(SrcReg, getKillRegState(KillSrc))
+                  .addImm(0);
+    MI->getOperand(0).setIsRenamable(MBBI->getOperand(0).isRenamable());
+    MI->getOperand(1).setIsRenamable(MBBI->getOperand(1).isRenamable());
     return;
   }
 
diff --git a/llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll b/llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll
new file mode 100644
index 00000000000000..d00e10dbd9c7f3
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll
@@ -0,0 +1,50 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc < %s -mtriple=riscv64 -mcpu=sifive-x280 -O3 | FileCheck %s
+
+
+define signext i32 @sum(ptr %a, i32 signext %n, i1 %prof.min.iters.check, <vscale x 8 x i1> %0, <vscale x 8 x i1> %1) {
+; CHECK-LABEL: sum:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a2, a2, 1
+; CHECK-NEXT:    beqz a2, .LBB0_4
+; CHECK-NEXT:  # %bb.1: # %for.body.preheader
+; CHECK-NEXT:    li a3, 0
+; CHECK-NEXT:  .LBB0_2: # %for.body
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    mv a2, a3
+; CHECK-NEXT:    lw a3, 0(a0)
+; CHECK-NEXT:    addi a0, a0, 4
+; CHECK-NEXT:    bnez a1, .LBB0_2
+; CHECK-NEXT:  # %bb.3: # %for.end
+; CHECK-NEXT:    mv a0, a2
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB0_4: # %vector.ph
+; CHECK-NEXT:    vsetvli a0, zero, e32, m4, ta, ma
+; CHECK-NEXT:    vmv.v.i v8, 0
+; CHECK-NEXT:    vmv.s.x v12, zero
+; CHECK-NEXT:    vsetivli zero, 1, e32, m4, ta, ma
+; CHECK-NEXT:    vredsum.vs v8, v8, v12, v0.t
+; CHECK-NEXT:    vmv.x.s a0, v8
+; CHECK-NEXT:    ret
+entry:
+  br i1 %prof.min.iters.check, label %for.body, label %vector.ph
+
+vector.ph:                                        ; preds = %entry
+  %2 = tail call i32 @llvm.vp.reduce.add.nxv8i32(i32 0, <vscale x 8 x i32> zeroinitializer, <vscale x 8 x i1> %0, i32 1)
+  br label %for.end
+
+for.body:                                         ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]
+  %red.05 = phi i32 [ %3, %for.body ], [ 0, %entry ]
+  %arrayidx = getelementptr i32, ptr %a, i64 %indvars.iv
+  %3 = load i32, ptr %arrayidx, align 4
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %exitcond.not = icmp eq i32 %n, 0
+  br i1 %exitcond.not, label %for.end, label %for.body
+
+for.end:                                          ; preds = %for.body, %vector.ph
+  %red.0.lcssa = phi i32 [ %2, %vector.ph ], [ %red.05, %for.body ]
+  ret i32 %red.0.lcssa
+}
+
+declare i32 @llvm.vp.reduce.add.nxv8i32(i32, <vscale x 8 x i32>, <vscale x 8 x i1>, i32)

topperc · 2024-04-24T04:53:47Z

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

+    auto MI = BuildMI(MBB, MBBI, DL, get(RISCV::ADDI), DstReg)
+                  .addReg(SrcReg, getKillRegState(KillSrc))
+                  .addImm(0);
+    MI->getOperand(0).setIsRenamable(MBBI->getOperand(0).isRenamable());


I'm a little concerned with checking the isRenamable flag from MBBI here when KillSrc is passed explicitly.

MBBI is, in general, not related to the copy, I think. According to the interface definition, it's just the insertion point.

topperc

Removing CopyOperands.Source->isRenamable() from isBackwardPropagatableCopy is another way to fix this without changing copyPhysReg. propagateDefs checks the renamable flag on the def it is propagating into. So the check in isBackwardPropagatableCopy might not be functionally needed.

BeMg · 2024-04-24T06:05:02Z

Removing CopyOperands.Source->isRenamable() from isBackwardPropagatableCopy is another way to fix this without changing copyPhysReg. propagateDefs checks the renamable flag on the def it is propagating into. So the check in isBackwardPropagatableCopy might not be functionally needed.

How about inherit the isRenamable during the TargetInstrInfo::lowerCopy?

llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll

BeMg · 2024-04-24T12:58:39Z

Remove -O3 from test

llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll

topperc · 2024-04-26T23:01:41Z

LGTM, but wait a few days for @arsenm and @efriedma-quic

efriedma-quic · 2024-04-29T21:17:13Z

llvm/lib/CodeGen/MachineCopyPropagation.cpp

@@ -1053,7 +1053,7 @@ void MachineCopyPropagation::BackwardCopyPropagateBlock(
    // Ignore non-trivial COPYs.
    std::optional<DestSourcePair> CopyOperands =
        isCopyInstr(MI, *TII, UseCopyInstr);
-    if (CopyOperands && MI.getNumOperands() == 2) {
+    if (CopyOperands) {


Obviously the immediate is irrelevant... but can we have implicit register operands here? Do we care if we do? It looks like isCopyInstrImpl() doesn't check for them.

I have a patch to fix that stuck on its 6th revert

efriedma-quic · 2024-04-29T21:43:28Z

llvm/lib/CodeGen/MachineCopyPropagation.cpp

@@ -983,7 +983,7 @@ static bool isBackwardPropagatableCopy(const DestSourcePair &CopyOperands,
  if (MRI.isReserved(Def) || MRI.isReserved(Src))
    return false;

-  return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
+  return CopyOperands.Source->isKill();


So... we know a copy is a copy, so it can't have inherent regalloc restrictions. But that isn't the only reason something might not be renamable, I think. See https://reviews.llvm.org/D43042 .

What do you think about fixing the copyPhysReg() API to explicitly pass in the "renamable" bit?

BeMg · 2024-08-26T12:45:17Z

Stack on #91179

Tail duplication will generate the redundant move before return. It is because the MachineCopyPropogation can't recognize COPY after post-RA pseudoExpand. This patch 1. Keep renamable after post-RA pseudoExpand 2. Let MachineCopyPropogation recognize `%0 = ADDI %1, 0` as COPY

BeMg · 2024-08-27T02:13:54Z

Rebase after #91179 landed.

wangpc-pp

LGTM.

BeMg requested a review from topperc April 24, 2024 03:13

llvmbot added backend:RISC-V llvm:regalloc labels Apr 24, 2024

BeMg mentioned this pull request Apr 24, 2024

[RISCV] precommit for redundant copy #89864

Closed

topperc changed the title ~~[RISCV] Remove redundant move from tail duplication~~ [RISCV][MCP] Remove redundant move from tail duplication Apr 24, 2024

topperc requested review from preames, efriedma-quic and arsenm April 24, 2024 04:52

topperc reviewed Apr 24, 2024

View reviewed changes

arsenm reviewed Apr 24, 2024

View reviewed changes

llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll Outdated Show resolved Hide resolved

BeMg force-pushed the makeMCP-know-ADDI-as-COPY branch from 9c0982b to 692628a Compare April 24, 2024 12:58

BeMg requested review from topperc and arsenm April 25, 2024 03:18

wangpc-pp reviewed Apr 25, 2024

View reviewed changes

llvm/test/CodeGen/RISCV/redundant-copy-from-tail-duplicate.ll Outdated Show resolved Hide resolved

efriedma-quic reviewed Apr 29, 2024

View reviewed changes

BeMg mentioned this pull request May 6, 2024

[TII][RISCV] Add renamable bit to copyPhysReg #91179

Merged

BeMg added 3 commits August 26, 2024 19:09

[RISCV] precommit for redundant copy

3799ad8

Restore isRenamable in isBackwardPropagatableCopy

9b4a3fd

BeMg force-pushed the makeMCP-know-ADDI-as-COPY branch from 7199c0c to 9b4a3fd Compare August 27, 2024 02:13

BeMg requested review from efriedma-quic and wangpc-pp August 27, 2024 06:16

wangpc-pp approved these changes Aug 27, 2024

View reviewed changes

BeMg merged commit 2def1c4 into llvm:main Aug 28, 2024
8 checks passed

houngkoungting mentioned this pull request Mar 25, 2025

[MachineCopyPropagation] Fix undefined physical register on SystemZ (#131478) #132927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV][MCP] Remove redundant move from tail duplication #89865

[RISCV][MCP] Remove redundant move from tail duplication #89865

Uh oh!

BeMg commented Apr 24, 2024 •

edited

Loading

Uh oh!

llvmbot commented Apr 24, 2024 •

edited

Loading

Uh oh!

topperc Apr 24, 2024 •

edited

Loading

Uh oh!

efriedma-quic Apr 24, 2024

Uh oh!

topperc left a comment •

edited

Loading

Uh oh!

BeMg commented Apr 24, 2024

Uh oh!

Uh oh!

BeMg commented Apr 24, 2024

Uh oh!

Uh oh!

topperc commented Apr 26, 2024

Uh oh!

efriedma-quic Apr 29, 2024

Uh oh!

arsenm Apr 30, 2024

Uh oh!

efriedma-quic Apr 29, 2024

Uh oh!

BeMg commented Aug 26, 2024

Uh oh!

BeMg commented Aug 27, 2024

Uh oh!

wangpc-pp left a comment

Uh oh!

Uh oh!

Uh oh!

[RISCV][MCP] Remove redundant move from tail duplication #89865

[RISCV][MCP] Remove redundant move from tail duplication #89865

Uh oh!

Conversation

BeMg commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc Apr 24, 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!

efriedma-quic Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

topperc left a comment • 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!

BeMg commented Apr 24, 2024

Uh oh!

Uh oh!

BeMg commented Apr 24, 2024

Uh oh!

Uh oh!

topperc commented Apr 26, 2024

Uh oh!

efriedma-quic Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

BeMg commented Aug 26, 2024

Uh oh!

BeMg commented Aug 27, 2024

Uh oh!

wangpc-pp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BeMg commented Apr 24, 2024 •

edited

Loading

llvmbot commented Apr 24, 2024 •

edited

Loading

topperc Apr 24, 2024 •

edited

Loading

topperc left a comment •

edited

Loading