[PATCH] D40298: [PowerPC] Merge register copies

[Tony Jiang via Phabricator via llvm-commits]

jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:325
 
+  auto isSameRegCopy = [](MachineInstr *I1, MachineInstr *I2) {
+    if (I1->getOpcode() == I2->getOpcode() &&
----------------
This lambda looks more like `isSameRegOperation`. From here we don't know they are both COPY Operation or not. It looks to me that is implied from the calling site. In that case, I would rename it to expression what it really does. Or we could change the implementation to match the current name. Either way is fine, as long as the name and what it does is consistent. And an assertion to make sure you only get COPY operation for this lambda may be necessary. 


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:450
+
+                for (auto &SuccMBB: PredMBB->successors())
+                  SuccMBB->addLiveIn(DstReg);
----------------
The format at line 441 is right, which is good, but we are missing space after `&SuccMBB` and before `:` here. Please also check all the other range based for loop format. Thanks!


================
Comment at: test/CodeGen/PowerPC/redundant_regcopy_3.mir:9
+  entry:
+    br i1 %b, label %exit, label %foo
+  
----------------
I tried to apply this patch and run this test case, but because this patch depends on another one, which I also tried to apply first, but that has conflicts with the latest trunk after applying. So I give up applying these patches.  The test case here looks weird to me. I may misunderstanding something, but the code block layout is not the same with the one in your implementation description. Basically,  bar block doesn't look like the successor of entry block. Do you mean  `br i1 %b, label %bar, label %foo` ? Or I misunderstand the test case? Can you explain a little bit here ? Thanks!


https://reviews.llvm.org/D40298