[AArch64] Avoid adding duplicate implicit operands when expanding pseudo insts.

[wwei]

Summary

When expanding pseudo insts, in order to create a new machine instr, we use BuildMI, which will add implicit operands by default.
And transferImpOps will also copy implicit operands from old ones. Finally, duplicate implicit operands are added to the same inst.
Sometimes this can cause correctness issues. Like below inst,

renamable $w18 = nsw SUBSWrr renamable $w30, renamable $w14, implicit-def dead $nzcv

After expanding, it will become

$w18 = SUBSWrs renamable $w13, renamable $w14, 0, implicit-def $nzcv, implicit-def dead $nzcv

A redundant implicit-def $nzcv is added, but the dead flag is missing.

[dmgreen]

Do we need to call transferImpOps here?
Or can we fix it directly to not add duplicate operands?

[wwei]

Yes, we need to call transferImpOps here, because it will copy the implict operand include the flag--like dead flag.
But BuildMI will ignore the flag, so the changes in this MR will use CreateMachineInstr(.... /*NoImplicit=*/true) to avoid adding duplicate implict operands
I will try to explain why there may have correct issues in postra scheduling later.
The test case is updated in : https://bugs.llvm.org/show_bug.cgi?id=51711

In D109069#2978111, @dmgreen wrote:

Do we need to call transferImpOps here?
Or can we fix it directly to not add duplicate operands?

[wwei]

The detailed explanation is：
Before post-ra scheduling, initial order of instructions：

A:    ... implicit-def dead $nzcv
B:    ... implicit-def $nzcv, implicit-def dead $nzcv
C:    ... implicit-def $nzcv
D:    ... implicit killed $nzcv    (use)

In the right way, the relationship should look like this after building the sched graph.

A     B                      
 \   /                   
   C   
   |             
   D

The deps: A->C(output dep)， B->C(output dep)， C->D(true dep)

Unfortunately, when the dependency graph is built, the graph we actually get :

A     B                      
     /                   
   C   
   |             
   D

The deps： A(no any deps with B/C/D) ， B->C(output dep)， C->D(true dep)， so after post-ra scheduling, we may get a wrong result:
B C A D, A will break the true dependency(C->D)

buildSchedGraph function will walk the list of instructions, from bottom moving up.
First, it will build dependency for D and C, C->D(true dep). Then for B, it will have B->C(output dep), we can see the code in addPhysRegDeps,

// Clear previous uses and defs of this register and its subergisters.
for (MCSubRegIterator SubReg(Reg, TRI, true); SubReg.isValid(); ++SubReg) {
  if (Uses.contains(*SubReg))
    Uses.eraseAll(*SubReg);
  if (!MO.isDead())
    Defs.eraseAll(*SubReg);
}

because B has an implicit-def $nzcv without dead, Defs list will clear all the previous define insts(C will be cleared). So when checking the dependence for A, it can‘t create output deps with C.
Finally, we can make sure the root cause is, inst B having a redundant implicit-def $nzcv, which is added by expanding pseudo pass.

[foad]

I guess addPhysRegDeps is not prepared to see two defs of the same physical register with inconsistent "dead" flags. Is it even valid MIR to have two defs of the same phys reg?

[dmgreen]

In D109069#2979191, @wwei wrote:

Yes, we need to call transferImpOps here, because it will copy the implict operand include the flag--like dead flag.

The kill/dead flags only need to be conservatively correct, as far as I understand. So we can remove them and let another pass re-calculate them as needed. Having them more correct is usually a good thing though.

In D109069#2981752, @foad wrote:

I guess addPhysRegDeps is not prepared to see two defs of the same physical register with inconsistent "dead" flags. Is it even valid MIR to have two defs of the same phys reg?

I would think it was invalid, yeah. At least in this case there doesn't seem to be much reason to keep both around.

It feels a little odd to me to bypass the BuildMI call so that we can copy the implicit defs from another instruction. As opposed to using/updating the ones that are added for this instruction. But from what I can tell they should be fine for all the instructions handled here, and I don't see anywhere else using transferImpOps that should have the same issues.

LGTM. Thanks for the fix.

[foad]

I tried to fix a similar issue in generic code in D100939 but got some push-back.

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

case AArch64::ORRXrr: {
     case AArch64::EONXrr:      Opcode = AArch64::EONXrs; break;
     case AArch64::EORWrr:      Opcode = AArch64::EORWrs; break;
     case AArch64::EORXrr:      Opcode = AArch64::EORXrs; break;
     case AArch64::ORNWrr:      Opcode = AArch64::ORNWrs; break;
     case AArch64::ORNXrr:      Opcode = AArch64::ORNXrs; break;
     case AArch64::ORRWrr:      Opcode = AArch64::ORRWrs; break;
     case AArch64::ORRXrr:      Opcode = AArch64::ORRXrs; break;
     }
-    MachineInstrBuilder MIB1 =
-        BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opcode),
-                MI.getOperand(0).getReg())
-            .add(MI.getOperand(1))
-            .add(MI.getOperand(2))
-            .addImm(AArch64_AM::getShifterImm(AArch64_AM::LSL, 0));
+    MachineFunction &MF = *MBB.getParent();
+    // Try to create new inst without implicit operands added.
+    MachineInstr *NewMI = MF.CreateMachineInstr(
+        TII->get(Opcode), MI.getDebugLoc(), /*NoImplicit=*/true);
+    MBB.insert(MBBI, NewMI);
+    MachineInstrBuilder MIB1(MF, NewMI);
+    MIB1.addReg(MI.getOperand(0).getReg(), RegState::Define)
     transferImpOps(MI, MIB1, MIB1);
     MI.eraseFromParent();
     return true;
   }
 
   case AArch64::LOADgot: {
     MachineFunction *MF = MBB.getParent();
     Register DstReg = MI.getOperand(0).getReg();

llvm/test/CodeGen/AArch64/expand-subs-pseudo.mir

+# RUN: llc -run-pass=aarch64-expand-pseudo -mtriple=aarch64-unknown-linux-gnu -o - %s | FileCheck %s
+
+---
+# CHECK-LABEL:  name: test
+# CHECK-LABEL:  bb.0:
+# CHECK:          $w5 = SUBSWrs renamable $w3, renamable $w2, 0, implicit-def dead $nzcv
+# CHECK-NEXT:     $w6 = SUBSWrs renamable $w5, renamable $w3, 0, implicit-def $nzcv
+# CHECK-NEXT:     RET undef $lr
+#
+name:            test
+alignment:       4
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $w5, $w6, $x2, $x3
+
+    renamable $w5 = nsw SUBSWrr renamable $w3, renamable $w2, implicit-def dead $nzcv
+    renamable $w6 = nsw SUBSWrr renamable $w5, renamable $w3, implicit-def $nzcv
+    RET_ReallyLR
+
+...