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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:160-162
+  /// Mapping registers to where they are used later. Key is actual registers
+  /// (not register units) Only used for Backward Propagation
+  DenseMap<MCRegister, SmallVector<MachineInstr *>> Uses;
----------------
The key is only MCRegisters, but invalidateRegister and similar functions only work with register units. How are Uses found so that they can be invalidated too?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:198-199
+
+    // Make sure when we remove Src registers from Copies, we remove the entire
+    // register used in the copy instruction.
+    for (MCRegister InvalidDefReg : RegsToInvalidate) {
----------------
Could you explain how this is different from the original code -- after all, the original implementation invalidated copes for the reguints for source and destinations of copies found.  I don't quite understand "entire register" -- is it the set of regunits, the aliases, or something else?


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:306-308
+  /// Track uses of registers that are candidates for backward copy propagation
+  void trackUse(MachineInstr *MI, const TargetRegisterInfo &TRI,
+                const TargetInstrInfo &TII) {
----------------
This would be a lot easier to understand if it had a motivating example of what it's doing -- see the top of MachineCopyPropagation.cpp for a few samples of what I mean.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:315
+        continue;
+      }
+      if (!MOUse.getReg()) {
----------------
clang-format? (And other single-statement-continues)


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:388-392
+    for (MCRegister InvalidReg : RegsToInvalidate) {
+      for (MCRegUnitIterator RUI(InvalidReg, &TRI); RUI.isValid(); ++RUI)
+        Copies.erase(*RUI);
+      Uses.erase(InvalidReg);
+    }
----------------
Shouldn't this be left to the rest of the machine-copy-propagate implementation -- after all, this method is called "trackUse", and it seems to be doing more than that.


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:935
+        if (User->isDebugValue()) {
+          MRI->updateDbgUsersToReg(Def, User);
+        } else {
----------------
I ran a stage2 RelWithDebInfo build of clang to test this (I was curious), and got an assertion failure in this function:

  llvm::MachineRegisterInfo::updateDbgUsersToReg(llvm::Register, ArrayRef<llvm::MachineInstr *>) const: Assertion `MI->getOperand(0).isReg()' failed.



================
Comment at: llvm/test/CodeGen/X86/machine-copy-prop.mir:139-149
+# CHECK-LABEL: name: copyprop3
+# CHECK: renamable $rbx = LEA64r $rax, 1, $noreg, 8, $noreg
+# CHECK-NEXT: renamable $rax = COPY renamable $rbx
+# CHECK-NEXT: NOOP implicit $rax
+name: copyprop3
+body: |
+  bb.0:
----------------
The rbp-to-rbx copy is deleted without this patch too, the only difference is that some of the registers change. I think this is fragile, and doesn't demonstrate the intention of the optimisation. Could you use a test body that
  a) deletes a COPY that wasn't already dead, and
  b) rewrites a non-COPY operand, another NOOP for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98659