[MachineCopyProp, SystemZ] "Using an undefined physical register"

[JonPsson1]

MachineVerifier reported *** Bad machine code: Using an undefined physical register ***.

git bisect shows

commit 2def1c4 (HEAD, refs/bisect/bad):

    [RISCV][MCP] Remove redundant move from tail duplication  (#89865)
    
    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

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index b34e0939d1c7..fab36f4858e0 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();

This changes the result of MachineCP on this test case like:

old <> failing

*** IR Dump After Machine Copy Propagation Pass (machine-cp) ***

  renamable $r14d = LLILL 0                                               |       renamable $r12d = LLILL 0
  renamable $r12d = COPY killed renamable $r14d, implicit-def $r12q       <
  renamable $r14l = LMux %fixed-stack.11, 4, $noreg :: (load (s32) fro            renamable $r14l = LMux %fixed-stack.11, 4, $noreg :: (load (s32) fro
  renamable $r14l = LLHRMux killed renamable $r14l                                renamable $r14l = LLHRMux killed renamable $r14l
  renamable $r3l = LMux killed renamable $r3d, 12, $noreg :: (load (s3            renamable $r3l = LMux killed renamable $r3d, 12, $noreg :: (load (s3
  renamable $r11d = LGFR killed renamable $r3l, implicit-def $r10q                renamable $r11d = LGFR killed renamable $r3l, implicit-def $r10q
  ST128 killed renamable $r10q, %stack.5, 0, $noreg :: (store (s128) i            ST128 killed renamable $r10q, %stack.5, 0, $noreg :: (store (s128) i
  renamable $r10q = COPY killed renamable $r12q                                   renamable $r10q = COPY killed renamable $r12q

*** IR Dump After Post-RA pseudo instruction expansion pass

  renamable $r14d = LLILL 0                                               |       renamable $r12d = LLILL 0
  $r12d = LGR killed $r14d, implicit-def $r12q                            <
  renamable $r14l = L $r15d, 380, $noreg :: (load (s32) from %fixed-st            renamable $r14l = L $r15d, 380, $noreg :: (load (s32) from %fixed-st
  $r14l = LLHR killed $r14l                                                       $r14l = LLHR killed $r14l
  renamable $r3l = L killed renamable $r3d, 12, $noreg :: (load (s32)             renamable $r3l = L killed renamable $r3d, 12, $noreg :: (load (s32) 
  renamable $r11d = LGFR killed renamable $r3l, implicit-def $r10q                renamable $r11d = LGFR killed renamable $r3l, implicit-def $r10q
  STG $r10d, $r15d, 184, $noreg, implicit $r10q :: (store (s128) into             STG $r10d, $r15d, 184, $noreg, implicit $r10q :: (store (s128) into 
  STG killed $r11d, $r15d, 192, $noreg, implicit killed $r10q :: (stor            STG killed $r11d, $r15d, 192, $noreg, implicit killed $r10q :: (stor
  $r10d = LGR killed $r12d, implicit $r12q                                        $r10d = LGR killed $r12d, implicit $r12q
  $r11d = LGR killed $r13d, implicit killed $r12q                                 $r11d = LGR killed $r13d, implicit killed $r12q

                                                                                  *** Bad machine code: Using an undefined physical register ***
                                                                                  - function:    t
                                                                                  - basic block: %bb.0  (0xade6dc0)
                                                                                  - instruction: $r11d = LGR killed $r13d, implicit killed $r12q
                                                                                  - operand 2:   implicit killed $r12q
                                                                                  LLVM ERROR: Found 1 machine code errors.

With the LGR (left), there is an implicit-def of $r12q.

In the failing version (right), I see MachineCP propagating a COPY, but while doing so dropping the implicit-def of $r12q:

 *** IR Dump After Greedy Register Allocator (greedy) ***:

528B      %53:gr64bit = LLILL 0
544B      undef %58.subreg_h64:gr128bit = COPY %53:gr64bit
636B      %57:gr128bit = COPY %58:gr128bit

 *** IR Dump After SystemZ Post Rewrite pass (systemz-post-rewrite) ***:

  renamable $r14d = LLILL 0
  renamable $r12d = COPY killed renamable $r14d, implicit-def $r12q
  renamable $r10q = COPY killed renamable $r12q

*** IR Dump After Machine Copy Propagation Pass (machine-cp) ***

  renamable $r12d = LLILL 0
  renamable $r10q = COPY killed renamable $r12q

*** IR Dump After Post-RA pseudo instruction expansion pass (postrapseudos) ***:

  renamable $r12d = LLILL 0
  $r10d = LGR killed $r12d, implicit $r12q
  $r11d = LGR killed $r13d, implicit killed $r12q
 

*** Bad machine code: $r12q is undefined in last LGR as $r13d never had a definition.

The MIR looks legal up until MachineCP, so maybe something is missing there?

tc_gr128CopyExp.tar.gz
llc -mcpu=z15 -O3 -disable-cgp -use-mbpi=false -greedy-reverse-local-assignment -aggressive-machine-cse -misched=ilpmin -verify-machineinstrs ./tc_gr128CopyExp.ll

@BeMg @arsenm @efriedma-quic @asb @jsji @vladimirradosavljevic @uweigand

[topperc]

Looks like we need to be aware of the implicit-def $r12q? Easiest fix is probably to check MI.getNumImplicitOperands() == 0 where we used to check getNumOperands() == 2

[houngkoungting]

Hi @JonPsson1 and @topperc , I'm a new contributor to LLVM and would like to work on this issue (#131478). I’ve been following the discussion and think I can implement the suggested fix by checking MI.getNumImplicitOperands() == 0 in MachineCopyPropagation.cpp. Please let me know if it’s okay for me to take this and submit a PR. Thanks!

[JonPsson1]

Hi @JonPsson1 and @topperc , I'm a new contributor to LLVM and would like to work on this issue (#131478). I’ve been following the discussion and think I can implement the suggested fix by checking MI.getNumImplicitOperands() == 0 in MachineCopyPropagation.cpp. Please let me know if it’s okay for me to take this and submit a PR. Thanks!

Hi @houngkoungting, that would be nice, thank you. Add some of the people from above in your description for review and also add the test case.

[houngkoungting]

Hi @JonPsson1 , I’m swamped with tasks this week, so I’m sorry for the delay. First off, I’ve removed '-aggressive-machine-cse,' and it should compile successfully now. I’m currently trying to tweak MachineCSE.cpp, but it’s not functioning yet. I’ll keep it. Thank for your waiting .

[JonPsson1]

Hi @JonPsson1 , I’m swamped with tasks this week, so I’m sorry for the delay. First off, I’ve removed '-aggressive-machine-cse,' and it should compile successfully now. I’m currently trying to tweak MachineCSE.cpp, but it’s not functioning yet. I’ll keep it. Thank for your waiting .

@houngkoungting ok, if you are swamped with work maybe we should see if someone else can take it. I think @topperc 's suggestion should be worth a try. @BeMg @arsenm @efriedma-quic @asb @jsji @vladimirradosavljevic

[houngkoungting]

HI @JonPsson1 .I think I’ve fixed the issue based on @topperc’s suggestion. I’ll test it later and let you know.

[JonPsson1]

HI @JonPsson1 .I think I’ve fixed the issue based on @topperc’s suggestion. I’ll test it later and let you know.

@houngkoungting You should open a separate PR with your patch so that it can be reviewed.

[JonPsson1]

This is still unhandled then, as @houngkoungting closed his/her PR (thanks for trying). @BeMg @arsenm @efriedma-quic @asb @jsji @vladimirradosavljevic @topperc

[houngkoungting]

Sorry, my initial code wasn’t perfect. Let me revise it and make the necessary improvements.

[JonPsson1]

Sorry, my initial code wasn’t perfect. Let me revise it and make the necessary improvements.

I think you really need to do just exactly what @topperc proposed and nothing else.

[JonPsson1]

Have not heard back from @houngkoungting in 2 weeks, so maybe @topperc or anyone else? @BeMg @arsenm @efriedma-quic @asb @jsji @vladimirradosavljevic

[JonPsson1]

Ping! Was this fixed by anyone? @dominik-steenken