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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Lambda functions are useful constructs in some cases, but if a patch consists of 3 lambda functions and 5 lines of code to just call them, it's an indication of an overuse of this construct. I would say that as general guidance, use a lambda when it:

1. Saves code repetition
2. Capturing values from the enclosing function is necessary
3. Functionality isn't needed outside of the function

**or** if you need to pass a functor to something and will likely just define it inline.

For this patch, neither of these 3 lambdas fulfill these requirements.
`getVRegDefMI()` is useful elsewhere and doesn't capture anything
`replaceLiWithAddi()` is a big lambda that doesn't save code repetition and I didn't notice it capturing anything (although I may have missed something in that big function)
`replaceAddWithCopy()` is useful elsewhere, doesn't save code repetition and only captures things which it shouldn't be concerned about



================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:72
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<MachineDominatorTree>();
+    MachineFunctionPass::getAnalysisUsage(AU);
----------------
You don't modify MDT, right? Why not `addPreserved()` as well?


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:78
   bool runOnMachineFunction(MachineFunction &MF) override {
+    MDT = &getAnalysis<MachineDominatorTree>();
     if (skipFunction(*MF.getFunction()))
----------------
Why initialize this if you're going to early exit? Seems more logical to put this in `initialize()`.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:355
+      case PPC::ADD8: {
+        auto getVRegDefMI = [&](MachineOperand *Op, MachineRegisterInfo *MRI) {
+          assert(Op && "Invalid Operand!");
----------------
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()`?


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:362
+          if (!TargetRegisterInfo::isVirtualRegister(Reg))
+            return (MachineInstr *)nullptr;
+
----------------
I don't think it's meaningful to cast `nullptr`.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:367
+
+        auto replaceLiWithAddi = [&](MachineOperand *DominatorOp,
+                                     MachineOperand *PhiOp) {
----------------
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?


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:369
+                                     MachineOperand *PhiOp) {
+          assert(PhiOp && DominatorOp && "Invalid Operand!");
+          MachineInstr *DefPhiMI = getVRegDefMI(PhiOp, MRI);
----------------
I don't think it's very useful to put both of these in a single assert. If you want to add an assert for both, I think they should be separate so it is clear which is null.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:408
+            // only replace the first one (in this specific example just replace
+            // the first %vreg8 from <BB#3>
+            if (LiMI->getOpcode() == PPC::ADDI ||
----------------
Everything after `In such case, ...` is kind of meaningless. We're in SSA here, there's no "first one" - the vreg is defined only once.
Please change that sentence to something like:
`So if we've already replaced the def instruction, skip.`


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:422
+                .addImm(LiImm); // restore the imm of LI
+            DEBUG(LiMI->dump()); // LiMI is actually AddiMI now
+          }
----------------
The comment is confusing. `LiMI` is a variable name you used, `AddiMI` is not. Please remove the comment - there's no reason to explain what you're doing here, it is clear that you're just dumping the new instruction.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:428
+
+        auto replaceAddWithCopy = [&](MachineOperand &PhiOp) {
+          DEBUG(dbgs() << "Optimizing ADD to COPY: ");
----------------
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.


================
Comment at: test/CodeGen/PowerPC/opt-li-add-to-addi.ll:4
+
+define i64 @testOptimizeLiAddToAddi(i64 %a) {
+; CHECK-LABEL: testOptimizeLiAddToAddi:
----------------
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)


https://reviews.llvm.org/D36734