Replace at most one dead register with zero register on aarch64

[yuyichao]

Summary

Fix https://llvm.org/bugs/show_bug.cgi?id=27081

As mentioned in my comment of the bug report, this seems to be fix the problem but it does make a few assumptions. This is currently assuming that no live dest can be zero register before this pass and there shouldn't be more than one dead register in other instructions.

I searched through the aarch64 instruction set manual and it seems that the load pair instructions are the only ones that can cause this trouble so maybe it is best to just check for those.

[t.p.northover]

I think it would be better to check MI->definesRegister(AArch64::XZR) || MI->definesRegister(AArch64::WZR) and continue. It seems to be a general principle that you can't write to even XZR twice in the same instruction, so that ought to be more robust.

We should also add a test to test/CodeGen/AArch64.

Cheers.

Tim.

[yuyichao]

I'll try that. Just to make sure, will it distinguish between sp and zr (e.g. ignore a stack load)

[t.p.northover]

Yep, they're completely different registers as far as LLVM is concerned.

Tim.

[jevinskie]

I'd like to get this resolved as well. I think it is related to PR 23209 (https://llvm.org/bugs/show_bug.cgi?id=23209).

[yuyichao]

Add a check for existing zero register use. There's actually tests for this already but the tests were wrong...

Is there a way to check if the pass keeps ldxp xzr, x.., [...] unchanged if the second destination is dead?

[yuyichao]

Ping. Anything else I should change?

[t.p.northover]

A MIR test could handle the already-existing "ldp xzr, ..." case, but I don't think that's essential here. Thanks for the revisions!

Tim.

[yuyichao]

So can this be merged?

[t.p.northover]

I think so.

Tim.

[yuyichao]

Could you please commit it for me please, since I don't have commit access.

Thx.

[t.p.northover]

Yep, it should be committed as r266206.

Tim.

[yuyichao]

Thanks.

lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp

       DEBUG(dbgs() << "    Ignoring, operand is frame index\n");
       continue;
     }
+    if (MI.definesRegister(AArch64::XZR) || MI.definesRegister(AArch64::WZR)) {
+      // It is not allowed to write to the same register (not even the zero
+      // register) twice in a single instruction.
+      DEBUG(dbgs() << "    Ignoring, XZR or WZR already used by the instruction\n");
+      continue;
+    }
     for (int i = 0, e = MI.getDesc().getNumDefs(); i != e; ++i) {
       MachineOperand &MO = MI.getOperand(i);
       if (MO.isReg() && MO.isDead() && MO.isDef()) {

         MO.setReg(NewReg);
         DEBUG(MI.print(dbgs()));
         ++NumDeadDefsReplaced;
+        // Only replace one dead register, see check for zero register above.
+        break;
       }
     }
   }

test/CodeGen/AArch64/arm64-atomic-128.ll

 ; CHECK-LABEL: atomic_store_seq_cst:
 ; CHECK-NOT: dmb
 ; CHECK: [[LABEL:.?LBB[0-9]+_[0-9]+]]:
-; CHECK: ldaxp xzr, xzr, [x2]
+; CHECK: ldaxp xzr, [[IGNORED:x[0-9]+]], [x2]
 ; CHECK: stlxp [[SUCCESS:w[0-9]+]], x0, x1, [x2]
 ; CHECK: cbnz [[SUCCESS]], [[LABEL]]
 ; CHECK-NOT: dmb

 ; CHECK-LABEL: atomic_store_release:
 ; CHECK-NOT: dmb
 ; CHECK: [[LABEL:.?LBB[0-9]+_[0-9]+]]:
-; CHECK: ldxp xzr, xzr, [x2]
+; CHECK: ldxp xzr, [[IGNORED:x[0-9]+]], [x2]
 ; CHECK: stlxp [[SUCCESS:w[0-9]+]], x0, x1, [x2]
 ; CHECK: cbnz [[SUCCESS]], [[LABEL]]
 ; CHECK-NOT: dmb

 ; CHECK-LABEL: atomic_store_relaxed:
 ; CHECK-NOT: dmb
 ; CHECK: [[LABEL:.?LBB[0-9]+_[0-9]+]]:
-; CHECK: ldxp xzr, xzr, [x2]
+; CHECK: ldxp xzr, [[IGNORED:x[0-9]+]], [x2]
 ; CHECK: stxp [[SUCCESS:w[0-9]+]], x0, x1, [x2]
 ; CHECK: cbnz [[SUCCESS]], [[LABEL]]
 ; CHECK-NOT: dmb