[PATCH] D29344: [AArch64] Extend redundant copy elimination pass to handle non-zero stores.

[Chad Rosier via Phabricator via llvm-commits]

mcrosier marked an inline comment as done.
mcrosier added a comment.

I added the test cases you suggested (and cleaned up the others).  In particular, I added the test (i.e., test 13) that has a 32 bit comparison and a move immediate that implicitly defines the upper bits.  We can definitely get into trouble here.  I added the necessary checks to make sure we don't remove the mov immediate in this case because that would leave the upper bits in some unknown state.



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:355
   // but should be okay since kill markers are being phased out.
   for (MachineInstr &MMI : make_range(FirstUse, PredMBB->end()))
     MMI.clearKillInfo();
----------------
gberry wrote:
> I don't think FirstUse is being calculated correctly in the case of the non-zero immediate.  E.g. if there are no COPYs to worry about, FirstUse should be the SUBS compare instruction since we want to remove the kill of the known value register in that case if it is present.  Your test case test10 should show this problem, though I'm not sure why -verfiy-machineinstrs isn't catching it.
> 
You are correct.  I've updated the logic and test cases to make sure the kill flags are removed.


https://reviews.llvm.org/D29344