[PATCH] D26111: AArch64: Schedule DeadRegisterDefinitionsPass before regalloc.

[Matthias Braun via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:92
+        continue;
+      // We should not have any relevant physreg defs that are replacable by
+      // zero before register allocation. So we just check for dead vreg defs.
----------------
gberry wrote:
> Perhaps you could add an assert here checking this?
I'd rather not, as it is technically legal to do so. This is merely an explanation why I did not add code to handle the physreg case; and I'd rather not add that code as it would be unused and hence untested today.


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:96
+      if (!TargetRegisterInfo::isVirtualRegister(Reg) ||
+          (!MO.isDead() && !MRI->use_nodbg_empty(Reg)))
         continue;
----------------
gberry wrote:
> Is this use_nodbg_empty check necessary?  Can we end up with virtual registers marked dead with dbg uses?
This is not about debug values (that function ignores any debug uses), but a way to detect dead definitions that do not have the Dead flag set.

I encountered several instances of dead defs without a dead flag set. I am not entirely sure whether the missing Dead flags are a bug or whether they need not be exact pre-regalloc. In any case you can see that DeadMachineInstructionElim does the same check to determine whether a def is dead.


================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:148
   TRI = MF.getSubtarget().getRegisterInfo();
-  bool Changed = false;
   DEBUG(dbgs() << "***** AArch64DeadRegisterDefinitions *****\n");
----------------
gberry wrote:
> Perhaps you should commit this bug fix (local Changed shadowing the member) separately?
ok.


https://reviews.llvm.org/D26111