Fix minor deficiency in MachineSink.

[markus]

Summary

Seems that physical register uses of registers that are MRI->isConstantPhysReg() incorrectly inhibited transformation (the comment in the code is talking about "zombie" define and NOT uses).

[markus]

ping

[StephenTozer]

Seems straightforward enough to me, the code block in question clearly only cares about preg defs for the purposes of sinking. LGTM, with one inline nit.

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

6

Remove double "is a".

[StephenTozer]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

2

Also just to add, does this need "-x mir"? I see some of the other llc tests on MIR files use it, but I don't know if it's actually required.

[StephenTozer]

Found one issue with the test (noted inline), but otherwise no issues.

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

36

I've just run the test, and it seems that this last line causes a parse error for llc; otherwise the test seems to work as written.

[markus]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

2

The majority of MIR tests depend on having llc detecting the input language from the filename extension and not using -x mir AFAICT.

36

Hmm, what exactly are you running then?
When I run

./bin/llvm-lit ../llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

it passes without issues.

[StephenTozer]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

36

The command:

python .\build\Release\bin\llvm-lit.py llvm\test\CodeGen\RISCV\MachineSink-implicit-x0.mir

fails with:

$ ":" "RUN: at line 1"
$ "d:\upstream-llvm\build\release\bin\llc.exe" "-mtriple=riscv32" "D:\upstream-llvm\llvm\test\CodeGen\RISCV\MachineSink-implicit-x0.mir" "-run-pass=machine-sink" "-o" "-"
# command stderr:
error: YAML:35:1: not a mapping
---
^~~

Deleting the dashes causes it to pass. This is with python 3.9.7 and an up-to-date build of LLVM from trunk (82b74363a9).

[StephenTozer]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

36

Also, this is a windows build - and I've just confirmed that when running the tests on linux, the final line does not affect the test, so this is presumably platform-dependent.

[StephenTozer]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

36

Alright, I've figured it out - when I applied the patch, the test file didn't contain the final newline, which was messing with YAML parsing. This should be fine as-is, apologies for the wasted time!

[markus]

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

36

Ah, right. I was not able to reproduce on linux but I will change it anyways since I see now that it is not present in other .mir tests. Not sure where I picked it up from.

No worries. Thanks for reviewing :)

llvm/lib/CodeGen/MachineSink.cpp

bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
   if (!SuccToSinkTo)
     return false;
 
   // If the instruction to move defines a dead physical register which is live
   // when leaving the basic block, don't move it because it could turn into a
   // "zombie" define of that preg. E.g., EFLAGS. (<rdar://problem/8030636>)
   for (unsigned I = 0, E = MI.getNumOperands(); I != E; ++I) {
     const MachineOperand &MO = MI.getOperand(I);
-    if (!MO.isReg()) continue;
+    if (!MO.isReg() || MO.isUse())
+      continue;
     Register Reg = MO.getReg();
     if (Reg == 0 || !Register::isPhysicalRegister(Reg))
       continue;
     if (SuccToSinkTo->isLiveIn(Reg))
       return false;
   }
 
   LLVM_DEBUG(dbgs() << "Sink instr " << MI << "\tinto block " << *SuccToSinkTo);

llvm/test/CodeGen/RISCV/MachineSink-implicit-x0.mir

+# RUN: llc -mtriple=riscv32 %s -run-pass=machine-sink -o - | FileCheck %s
+
+# Verify that sinking of '%20:gpr = LUI 1, implicit $x0' is not inhibited by
+# the implicit use of '$x0'.
+# Register '$x0' is a 'MRI->isConstantPhysReg()' on RISCV and such uses should
+# not inhibit sinking transformation even though they are livein to the block
+# they are to be sunk into (inhibit under such conditions should only happen
+# for defines).
+
+---
+name:            f
+tracksRegLiveness: true
+body:             |
+  ; CHECK-LABEL: bb.1:
+  ; CHECK-NEXT:   successors: %bb.3(0x80000000)
+  ; CHECK-NEXT:   liveins: $x0
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[LUI:%[0-9]+]]:gpr = LUI 1, implicit $x0
+  bb.0:
+    liveins: $x10
+    %10:gpr = COPY $x10
+    %20:gpr = LUI 1, implicit $x0
+    BEQ %10, %10, %bb.2
+    PseudoBR %bb.1
+  bb.1:
+    liveins: $x0
+    %30:gpr = ADDI %20, 5
+    PseudoBR %bb.3
+  bb.2:
+    PseudoBR %bb.3
+  bb.3:
+    PseudoRET
+...