[PATCH] D80886: [PHIElimination] Fix the killed flag for LowerPHINode()

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang added inline comments.


================
Comment at: llvm/lib/CodeGen/PHIElimination.cpp:327
 
-      // When we are reusing the incoming register, it may already have been
-      // killed in this block. The old kill will also have been inserted at
-      // AfterPHIsIt, so it appears before the current PHICopy.
-      if (reusedIncoming)
-        if (MachineInstr *OldKill = VI.findKill(&MBB)) {
-          LLVM_DEBUG(dbgs() << "Remove old kill from " << *OldKill);
-          LV->removeVirtualRegisterKilled(IncomingReg, *OldKill);
-          LLVM_DEBUG(MBB.dump());
+      MachineInstr *OldKill = VI.findKill(&MBB);
+      bool IsPHICopyAfterOldKill = false;
----------------
bjope wrote:
> This findKill was guarded by `if (reusedIncoming)` in the past. I guess it won't hurt to keep it that way (to avoid any unpleasant surprises with build speeds, not that I think it will cause any big problems but OldKill is only of interest if reusedIncoming is true).
I will use reusedIncoming to guard the OldKill.


================
Comment at: llvm/test/CodeGen/PowerPC/phi-eliminate.mir:174
   ; CHECK:   %38:g8rc_and_g8rc_nox0 = COPY killed %59
-  ; CHECK:   %37:gprc = COPY killed %57
+  ; CHECK:   %37:gprc = COPY %57
   ; CHECK:   %36:gprc = COPY killed %58
----------------
bjope wrote:
> Could be seen as an unrelated comment, but I've always wondered why PHIElimination swap the order of the defs when converting PHIs to COPYs. I guess it could be easier to deal with the liveness attributes such as "killed" is not reversing the order (but I guess those target hooks, that makes it impossible to know where the COPY is inserted, would still be a nuisance).
In fact, for general cases without target hook, reversing the order is easier.

If it's not reversed order, you can think the IsPHICopyAfterOldKill is always true, I need remove the killed flag for OldKill and add killed for the coming reg. The killed flag added for the OldKill is not needed.

But if it's reversed order, you can think the IsPHICopyAfterOldKill is always false, I only need consider(line 362) whether I should add killed for the coming reg,  



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80886/new/

https://reviews.llvm.org/D80886