[MachineCopyPropagation] Remove logic to recognise and delete no-op moves produced after forwarded uses #167336
Conversation
…oves produced after forwarded uses As reported in <llvm#166870>, some copies with src==reg are not no-ops, e.g. when self-assigning a w-reg on AArch64 which will zero-extend the corresponding x register. Revert in order to fix the issue. We may revisit whether the optimisation can be made safe at a later point. Reverts dffbc03.
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesAs reported in <#166870>, some copies with src==reg are not no-ops, e.g. when self-assigning a w-reg on AArch64 which will zero-extend the corresponding x register. Revert in order to fix the issue. We may revisit whether the optimisation can be made safe at a later point. Reverts dffbc03. Full diff: https://github.com/llvm/llvm-project/pull/167336.diff 5 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index ea08365810a29..187bff78f236f 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -937,16 +937,6 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
if (CopyOperands) {
Register RegSrc = CopyOperands->Source->getReg();
Register RegDef = CopyOperands->Destination->getReg();
- // It's possible that the previous transformations have resulted in a
- // no-op register move (i.e. one where source and destination registers
- // are the same and are not referring to a reserved register). If so,
- // delete it.
- if (RegSrc == RegDef && !MRI->isReserved(RegSrc)) {
- MI.eraseFromParent();
- NumDeletes++;
- Changed = true;
- continue;
- }
if (!TRI->regsOverlap(RegDef, RegSrc)) {
// Copy is now a candidate for deletion.
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/constbarrier-rv32.ll b/llvm/test/CodeGen/RISCV/GlobalISel/constbarrier-rv32.ll
index b24ea9ec1561e..3c617f9854761 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/constbarrier-rv32.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/constbarrier-rv32.ll
@@ -32,9 +32,12 @@ define void @constant_fold_barrier_i128(ptr %p) {
; RV32-NEXT: mv a6, a1
; RV32-NEXT: seqz a7, a1
; RV32-NEXT: and a1, a7, a1
+; RV32-NEXT: mv a1, a1
; RV32-NEXT: mv a7, a1
; RV32-NEXT: seqz a3, a1
; RV32-NEXT: and a1, a3, a1
+; RV32-NEXT: mv a1, a1
+; RV32-NEXT: mv a1, a1
; RV32-NEXT: sw a2, 0(a0)
; RV32-NEXT: sw a6, 4(a0)
; RV32-NEXT: sw a7, 8(a0)
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/div-by-constant.ll b/llvm/test/CodeGen/RISCV/GlobalISel/div-by-constant.ll
index 225ceed9627b7..5f61ee2d02d24 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/div-by-constant.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/div-by-constant.ll
@@ -103,15 +103,18 @@ define i64 @udiv64_constant_no_add(i64 %a) nounwind {
; RV32-NEXT: mulhu a1, a1, a2
; RV32-NEXT: add a5, a5, a6
; RV32-NEXT: mv t0, t1
+; RV32-NEXT: mv a1, a1
; RV32-NEXT: sltu a4, a5, a6
; RV32-NEXT: add a5, a5, a7
; RV32-NEXT: sltu a6, t1, t1
; RV32-NEXT: sltiu t1, t1, 0
; RV32-NEXT: add t0, t0, t2
+; RV32-NEXT: mv a1, a1
; RV32-NEXT: sltu a2, a5, a7
; RV32-NEXT: add a6, a6, t1
; RV32-NEXT: sltu a5, t0, t2
; RV32-NEXT: add t0, t0, a0
+; RV32-NEXT: mv a1, a1
; RV32-NEXT: add a2, a4, a2
; RV32-NEXT: add a5, a6, a5
; RV32-NEXT: sltu a0, t0, a0
@@ -155,6 +158,7 @@ define i64 @udiv64_constant_add(i64 %a) nounwind {
; RV32-NEXT: mulhu a7, a0, a2
; RV32-NEXT: mulhu t2, a1, a3
; RV32-NEXT: mv t1, t2
+; RV32-NEXT: mv t1, t1
; RV32-NEXT: mul t2, a1, a3
; RV32-NEXT: mulhu a2, a1, a2
; RV32-NEXT: mulhu a3, a0, a3
diff --git a/llvm/test/CodeGen/RISCV/machine-copyprop-noop-removal.mir b/llvm/test/CodeGen/RISCV/machine-copyprop-noop-removal.mir
index d739537b50d05..293b15bf9d25e 100644
--- a/llvm/test/CodeGen/RISCV/machine-copyprop-noop-removal.mir
+++ b/llvm/test/CodeGen/RISCV/machine-copyprop-noop-removal.mir
@@ -1,8 +1,11 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -o - %s -mtriple=riscv64 -run-pass=machine-cp -mcp-use-is-copy-instr | FileCheck %s
-## This test was added to capture a case where MachineCopyPropagation risks
-## leaving a no-op register move (add, x0, reg).
+## This test was added to capture a case where MachineCopyPropagation may
+## leave a no-op register move (add reg, x0, reg).
+## Due to the bug reported in
+## <https://github.com/llvm/llvm-project/issues/166870>, we are not currently
+## able to optimize this case.
---
name: ham
@@ -21,6 +24,7 @@ body: |
; CHECK-NEXT: liveins: $x10
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x11 = ADDI $x0, 0
+ ; CHECK-NEXT: renamable $x10 = ADDI killed renamable $x10, 0
; CHECK-NEXT: BEQ renamable $x10, $x0, %bb.4
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
diff --git a/llvm/test/CodeGen/RISCV/sextw-removal.ll b/llvm/test/CodeGen/RISCV/sextw-removal.ll
index b155feab9b4d9..9f326280885b5 100644
--- a/llvm/test/CodeGen/RISCV/sextw-removal.ll
+++ b/llvm/test/CodeGen/RISCV/sextw-removal.ll
@@ -1352,6 +1352,7 @@ define signext i32 @sextw_sh2add(i1 zeroext %0, ptr %1, i32 signext %2, i32 sign
; NOREMOVAL-LABEL: sextw_sh2add:
; NOREMOVAL: # %bb.0:
; NOREMOVAL-NEXT: sh2add a2, a2, a3
+; NOREMOVAL-NEXT: mv a2, a2
; NOREMOVAL-NEXT: beqz a0, .LBB22_2
; NOREMOVAL-NEXT: # %bb.1:
; NOREMOVAL-NEXT: sw a2, 0(a1)
|
|
Should we add a test for AArch64 to prevent reintroducing this? |
github-project-automation
bot
moved this from Needs Backport PR
to Done
in LLVM Release Status
|
/cherry-pick a314b3b |
|
/pull-request #167526 |
…oves produced after forwarded uses (llvm#167336) As reported in <llvm#166870>, some copies with src==reg are not no-ops, e.g. when self-assigning a w-reg on AArch64 which will zero-extend the corresponding x register. Revert in order to fix the issue. We may revisit whether the optimisation can be made safe at a later point. Reverts dffbc03. Fixes llvm#166870. (cherry picked from commit a314b3b)
Hi - I would be interested in seeing an example of where this goes wrong if we have one. I know it can be difficult to reproduce, but I was interested in if there was something we could do to make it more reliable. |
Alex added a test, see |
|
In this code: Where the mov w8, w8 was being removed previously? The upper bits are already zero from the previous mov, so it should not be an issue in itself. I would guess that the original issue, whatever it was, might be difficult to pin down and replicate but I might give it a try if I get time. |
I guess I over-reduced it. Checking for where we get a self-move to a w reg in one toolchain build but not the other captures the difference in compilation behaviour, but you're right it's not actually exhibiting a bug. I can take another look if we can get something that better. |
4 participants
As reported in #166870, some copies with src==reg are not no-ops, e.g. when self-assigning a w-reg on AArch64 which will zero-extend the corresponding x register.
Revert in order to fix the issue. We may revisit whether the optimisation can be made safe at a later point.
Reverts dffbc03.
Fixes #166870.