[MIR-Canon] Hardening propagateLocalCopies.

[plotfi]

Summary

[MIR-Canon] Hardening propagateLocalCopies.

- If there is no register class for a COPY's src or dst, bail.
- Fixes uses iterator clobbering bug.

[compnerd]

llvm/lib/CodeGen/MIRCanonicalizerPass.cpp

345

Can you explain why the type along isn't sufficient

346

I think that if (!MRI.getRegClassOrNull(Dst)) is more LLVM style

[plotfi]

llvm/lib/CodeGen/MIRCanonicalizerPass.cpp

345

Because in some cases like:

%namedVReg4353:sreg_32_xm0 = S_MOV_B32 0
%namedVReg1352:vgpr_32 = COPY %namedVReg4353
...
BUFFER_STORE_DWORD_ADDR64 %namedVReg1352, %namedVReg1353, %namedVReg1354, 0, 0, 0, 0, 0, 0, implicit $exec

Folding the copy causes a verifier error even though the types are the same:

  %namedVReg4353:sreg_32_xm0 = S_MOV_B32 0
  ...
  BUFFER_STORE_DWORD_ADDR64 %namedVReg4353:sreg_32_xm0, %namedVReg1353:vreg_64, %namedVReg1354:sgpr_128, 0, 0, 0, 0, 0, 0, implicit $exec
  S_ENDPGM 0

*** Bad machine code: Illegal virtual register for instruction ***
- function:    foo
- basic block: %bb.0  (0x559c6b1fa878)
- instruction: BUFFER_STORE_DWORD_ADDR64 %namedVReg4353:sreg_32_xm0, %namedVReg1353:vreg_64, %namedVReg1354:sgpr_128, 0, 0, 0, 0, 0, 0, implicit $exec
- operand 0:   %namedVReg4353:sreg_32_xm0
Expected a VGPR_32 register, but got a SReg_32_XM0 register
LLVM ERROR: Found 1 machine code errors.

I think the right way is only to fold on matching types if regbankselect has not happened (in the case where there is no register class), which I will probably add in a different commit.

[plotfi]

Adding some better comments. Addressing @compnerd's style suggestion.

[plotfi]

whitespace

[plotfi]

test case enhancement

[compnerd]

Thanks for adding the test case!

[spatel]

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

[spatel]

In D62713#1535547, @spatel wrote:

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

When it fails, the verbose output says something like this:

: 'RUN: at line 1';   /Users/spatel/GitHub/llvm-project/release/bin/llc -o -  -march=amdgcn  -run-pass mir-canonicalizer  -x mir /Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir | /Users/spatel/GitHub/llvm-project/release/bin/FileCheck /Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir
--
Exit Code: 1

Command Output (stderr):
--
/Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir:5:15: error: CHECK-NEXT: is not on the line after the previous match
# CHECK-NEXT: %namedVReg1358:vgpr_32 = COPY %namedVReg1361
              ^
<stdin>:77:2: note: 'next' match was here
 %namedVReg1358:vgpr_32 = COPY %namedVReg1361
 ^
<stdin>:75:46: note: previous match ended here
 %namedVReg1352:vgpr_32 = COPY %namedVReg4353
                                             ^
<stdin>:76:1: note: non-matching line after previous match is here
 %namedVReg1353:vreg_64 = REG_SEQUENCE %namedVReg4354, %subreg.sub0, %namedVReg1352, %subreg.sub1

[plotfi]

Very strange that the non-determinism is introduced after fixing the iterator invalidation. I will investigate and put something together tomorrow.

[krytarowski]

This change makes NetBSD buildbot flaky.

http://lab.llvm.org:8011/builders/netbsd-amd64/builds/20439/steps/run%20unit%20tests/logs/FAIL%3A%20LLVM%3A%3Amir-canon-multi.mir

Please fix.

[plotfi]

I have a fix. Will land shortly.

[plotfi]

r363012 should address netbsd and the other bot failures.

llvm/lib/CodeGen/MIRCanonicalizerPass.cpp

if (!MI->getOperand(1).isReg())
       continue;
 
     const unsigned Dst = MI->getOperand(0).getReg();
     const unsigned Src = MI->getOperand(1).getReg();
 
     if (!TargetRegisterInfo::isVirtualRegister(Dst))
       continue;
     if (!TargetRegisterInfo::isVirtualRegister(Src))
       continue;
+    // Not folding COPY instructions if regbankselect has not set the RCs.
+    // Why are we only considering Register Classes? Because the verifier
+    // sometimes gets upset if the register classes don't match even if the
+    // types do. A future patch might add COPY folding for matching types in
+    // pre-registerbankselect code.
+    if (!MRI.getRegClassOrNull(Dst))
+      continue;
     if (MRI.getRegClass(Dst) != MRI.getRegClass(Src))
       continue;
 
-    for (auto UI = MRI.use_begin(Dst); UI != MRI.use_end(); ++UI) {
-      MachineOperand *MO = &*UI;
+    std::vector<MachineOperand *> Uses;
+    for (auto UI = MRI.use_begin(Dst); UI != MRI.use_end(); ++UI)
+      Uses.push_back(&*UI);
+    for (auto *MO : Uses)
       MO->setReg(Src);
-      Changed = true;
-    }
 
+    Changed = true;
     MI->eraseFromParent();
   }
 
   return Changed;
 }
 
 /// Here we find our candidates. What makes an interesting candidate?
 /// An candidate for a canonicalization tree root is normally any kind of

llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir

 # RUN: llc -o -  -march=amdgcn  -run-pass mir-canonicalizer  -x mir %s | FileCheck %s
 
+# CHECK: %namedVReg4354:vgpr_32 = COPY $vgpr0
 # CHECK: %namedVReg1352:vgpr_32 = COPY %namedVReg4353
-# CHECK: %namedVReg1359:vgpr_32 = COPY %namedVReg1362
-# CHECK: %namedVReg1360:vgpr_32 = COPY %namedVReg1363
+# CHECK-NEXT: %namedVReg1358:vgpr_32 = COPY %namedVReg1361
+# CHECK-NEXT: %namedVReg1359:vgpr_32 = COPY %namedVReg1362
+# CHECK-NEXT: %namedVReg1353:vreg_64 = REG_SEQUENCE %namedVReg4354, %subreg.sub0, %namedVReg1352, %subreg.sub1
+# CHECK-NEXT: %namedVReg1354:sgpr_128 = REG_SEQUENCE %namedVReg4354, %subreg.sub0, %namedVReg1352, %subreg.sub1, %namedVReg1358, %subreg.sub2, %namedVReg1359, %subreg.sub3
+# This tests for the itereator invalidation fix (reviews.llvm.org/D62713)
+# CHECK-NEXT: BUFFER_STORE_DWORD_ADDR64 %namedVReg1352, %namedVReg1353, %namedVReg1354, 0, 0, 0, 0, 0, 0, implicit $exec
 ...
 ---
 name: foo
 body:             |
   bb.0:
     %10:sreg_32_xm0 = S_MOV_B32 61440
     %11:sreg_32_xm0 = S_MOV_B32 0
     %3:vgpr_32 = COPY $vgpr0