[PATCH] D35075: [AArch64] Redundant copy elimination - remove more zero copies.

[Geoff Berry via Phabricator via llvm-commits]

gberry added a comment.

A few more comments (still looking)...



================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:8
 //
 // This pass removes unnecessary zero copies in BBs that are targets of
 // cbz/cbnz instructions. For instance, the copy instruction in the code below
----------------
This comment needs some updating as well: not just zero copies are removed and not just cbz/cbnz branches are handled.


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:16
+//
+// Also, if the cmp defines a register we can remove a zero copy in some cases.
+//  BB#0:
----------------
Change this to 'if the flag setting opcode defined a register'


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:37
 // the comparison if the compile time regression is acceptable.
 //
 //===----------------------------------------------------------------------===//
----------------
Not really related, but can you add a FIXME to look into handling CCMP opcodes as well?


================
Comment at: lib/Target/AArch64/AArch64RedundantCopyElimination.cpp:182
       // Must not be a symbolic immediate.
       if (!PredI.getOperand(2).isImm())
+        return false;
----------------
Can you move this check down into the if(!ClobberedRegs[SrcReg] ...) check so that we don't miss cases where the destination is not WZR/XZR but the immediate is symbolic?


================
Comment at: test/CodeGen/AArch64/machine-zero-copy-remove.mir:1
+# RUN: llc -mtriple=aarch64--linux-gnu -run-pass=aarch64-copyelim %s -verify-machineinstrs -o - | FileCheck %s
+---
----------------
It seems like overkill to test every opcode here.  I think more negative test coverage would be better.


https://reviews.llvm.org/D35075