[PATCH] D36734: [PowerPC Peephole] Constants into a join add, use ADDI over LI/ADD.

[Tony Jiang via Phabricator via llvm-commits]

jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:355
+      case PPC::ADD8: {
+        auto getVRegDefMI = [&](MachineOperand *Op, MachineRegisterInfo *MRI) {
+          assert(Op && "Invalid Operand!");
----------------
nemanjai wrote:
> It isn't clear to me why the default capture? Are you actually capturing something and I just missed it?
> In any case, I think this function would be useful elsewhere in this file. Can it be a static function in this pass and called something like `getVregDefOrNull()`?
Changed to a static function call for this lambda function.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:362
+          if (!TargetRegisterInfo::isVirtualRegister(Reg))
+            return (MachineInstr *)nullptr;
+
----------------
nemanjai wrote:
> I don't think it's meaningful to cast `nullptr`.
Without that cast, the compiler couldn't figure out the return type for the function, but after changing to a static function, this won't be needed anymore.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:367
+
+        auto replaceLiWithAddi = [&](MachineOperand *DominatorOp,
+                                     MachineOperand *PhiOp) {
----------------
nemanjai wrote:
> Why implement this as a large lambda function that is (in effect) called only once (i.e. the two calls are symmetrical)? Would it not be much clearer and more readable to decide on the direction of the replacement early (i.e. is Op1 or Op2 checked for being a dominator, etc.) and then just transform instructions if possible?
I thought this is more clear actually, we are trying both the two cases in the calling part of this function replaceLiWithAddi. It is either Op1 is the DominatorOp and Op2 is the PhiOp or it is the other way:  Op2 is the DominatorOp and Op1 is the PhiOp, we don't have to handle the logic which is which ( by either set a flag or swap the two operands). But I may be wrong,  if that is a quite easy way  to decide on the direction of the replacement early and then just call the function replaceLiWithAddi once, then I can implement it and get rid of the lambda replaceLiWithAddi here.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:428
+
+        auto replaceAddWithCopy = [&](MachineOperand &PhiOp) {
+          DEBUG(dbgs() << "Optimizing ADD to COPY: ");
----------------
nemanjai wrote:
> Again, this function has wider utility than this. It should probably be a static function available everywhere in this pass. And it should take an instruction that it is to replace with a copy.
I would leave this one as a lambda function to save several parameter passing if there is no strong objection (otherwise you have to pass ToErase / Simplified / MBB / MI. And  Simplified  is bool parameter, which the community also doesn't like)


================
Comment at: test/CodeGen/PowerPC/opt-li-add-to-addi.ll:4
+
+define i64 @testOptimizeLiAddToAddi(i64 %a) {
+; CHECK-LABEL: testOptimizeLiAddToAddi:
----------------
nemanjai wrote:
> This test case is inadequate. Your code does more than just this:
> 1. Add a case where the same vreg can come from multiple blocks as outlined in your code
> 2. Add a case where the PHI has more than two incoming vregs (i.e. more LI instructions that will be replaced)
I added one test for case 2. but for case 1, it is not trivial to create one test case , the reduced test case for it is too large, we don't want to commit that in.


https://reviews.llvm.org/D36734