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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:428
+
+        auto replaceAddWithCopy = [&](MachineOperand &PhiOp) {
+          DEBUG(dbgs() << "Optimizing ADD to COPY: ");
----------------
jtony wrote:
> 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)
You've clearly not read my comment about this function. The arguments you're making for keeping this a lambda are that the things you currently capture would have to become parameters. As I've stated - **this function is capturing things it shouldn't be concerned with**.
`ToErase` and `Simplified` have nothing to do with what this function should do. This function should simply take a `MachineInstr` and replace it with a `PPC::COPY`. We do that all over the place in this pass. It would be good to have it in a function.
Then it is the caller that should be responsible for setting those.

If all the places where we replace an instruction with a `PPC::COPY` are in the same function, this can remain a lambda and I suppose it can capture these things, but it should be defined outside the `switch` and be available to the entire function.


https://reviews.llvm.org/D36734