[PATCH] D40298: [PowerPC] Merge register copies

Hiroshi Inoue via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 22 00:14:42 PST 2017


inouehrs added inline comments.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:325
 
+  auto isSameRegCopy = [](MachineInstr *I1, MachineInstr *I2) {
+    if (I1->getOpcode() == I2->getOpcode() &&
----------------
jtony wrote:
> 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. 
I updated to use `isRegCopy` helper to access source and destination registers.


================
Comment at: lib/Target/PowerPC/PPCRegCopyElim.cpp:450
+
+                for (auto &SuccMBB: PredMBB->successors())
+                  SuccMBB->addLiveIn(DstReg);
----------------
jtony wrote:
> 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!
Fixed. Thanks!


================
Comment at: test/CodeGen/PowerPC/redundant_regcopy_3.mir:9
+  entry:
+    br i1 %b, label %exit, label %foo
+  
----------------
jtony wrote:
> 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!
I updated. IR and MIR were mismatch.


https://reviews.llvm.org/D40298





More information about the llvm-commits mailing list