[PATCH] D98659: [MachineCopyPropagation] Do more backward copy propagations

Wed Mar 24 10:14:15 PDT 2021

jmorse added a comment.

There isn't a lot of difference to how debug-info is updated, as far as I can see, but the summary says a fault is resolved. Is this patch just masking the debug-info issue by doing more propagation? (I might have missed something in the implementation).

For the codegen change -- I would suggest this additional feature deserves a dedicated MIR test. All the tests modified are IR-to-assembly, where unrelated changes to other parts of the compiler might obscure this feature being accidentally disabled or otherwise modified. A MIR test for this specific behaviour in this specific pass will guarantee it isn't broken in the future. Plus, it'll make it easier for reviewers and post-review-readers to understand the patch.

================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:283-284

-  MachineInstr *findAvailBackwardCopy(MachineInstr &I, MCRegister Reg,
-                                      const TargetRegisterInfo &TRI) {
+  std::pair<MachineInstr *, SmallVector<MachineInstr *>>
+  findAvailBackwardCopy(MachineInstr &I, MCRegister Reg,
+                        const TargetRegisterInfo &TRI) {
----------------
Performance nit -- is it safe to return a `SmallVector *` in the pair? That avoids needless copying and potential allocations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659