[PATCH] D45394: Canonical Copy Propagation

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:318
+
+  std::vector<MachineInstr*> copies;
+  for (auto II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {
----------------
Capitalize variables and clang-format, please.


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:319
+  std::vector<MachineInstr*> copies;
+  for (auto II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {
+    if (II->isCopy())
----------------
for (MachineInstr &MI : MBB->instrs()), maybe?


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:320-321
+  for (auto II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {
+    if (II->isCopy())
+      copies.push_back(&*II);
+  }
----------------
Do we need to be careful about register classes here to preserve semantics?


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:324
+
+  for (auto MI : copies) {
+
----------------
Better to write "MachineInstr *" rather than auto here, it's not much longer and the clarity is worth it


================
Comment at: lib/CodeGen/MIRCanonicalizerPass.cpp:339-347
+    std::vector<MachineOperand *> RenameMOs;
+    for (auto UI = MRI.use_begin(Dst); UI != MRI.use_end(); ++UI) {
+      RenameMOs.push_back(&*UI);
+    }
+
+    for (auto *MO : RenameMOs) {
+      Changed = true;
----------------
Is the RenameMOs vector necessary? Why not just setReg in the loop over uses? Also I believe we can use range-for with MRI.uses() here to simplify.


================
Comment at: test/CodeGen/MIR/AArch64/mirCanonCopyCopyProp.mir:109-111
+    ;CHECK: %namedVReg1418:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)
+    ;CHECK: $w0 = COPY %namedVReg1418
+    ;CHECK: RET_ReallyLR implicit $w0
----------------
I feel like this test can be made much shorter. Can't we do this with just two or three instructions?


Repository:
  rL LLVM

https://reviews.llvm.org/D45394