This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jtony on Aug 15 2017, 6:26 AM.

Details

Summary

Two blocks prior to the join each perform an li and the the join block has an add using the initialized register. Optimize each predecessor block to instead use addi and delete the li's and add.

Diff Detail

Repository
rL LLVM

Event Timeline

jtony created this revision.Aug 15 2017, 6:26 AM
nemanjai edited edge metadata.Aug 20 2017, 1:17 AM

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

lib/Target/PowerPC/PPCMIPeephole.cpp
72 ↗(On Diff #111152)

You don't modify MDT, right? Why not addPreserved() as well?

78 ↗(On Diff #111152)

Why initialize this if you're going to early exit? Seems more logical to put this in initialize().

355 ↗(On Diff #111152)

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()?

362 ↗(On Diff #111152)

I don't think it's meaningful to cast nullptr.

367 ↗(On Diff #111152)

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?

369 ↗(On Diff #111152)

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.

408 ↗(On Diff #111152)

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.

422 ↗(On Diff #111152)

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.

428 ↗(On Diff #111152)

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.

test/CodeGen/PowerPC/opt-li-add-to-addi.ll
4 ↗(On Diff #111152)

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)
jtony updated this revision to Diff 112591.Aug 24 2017, 12:29 PM
jtony marked 10 inline comments as done.

Address the comments from Nemanja.

lib/Target/PowerPC/PPCMIPeephole.cpp
355 ↗(On Diff #111152)

Changed to a static function call for this lambda function.

362 ↗(On Diff #111152)

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.

367 ↗(On Diff #111152)

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.

428 ↗(On Diff #111152)

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)

test/CodeGen/PowerPC/opt-li-add-to-addi.ll
4 ↗(On Diff #111152)

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.

jtony marked 5 inline comments as done.Aug 24 2017, 12:31 PM
nemanjai added inline comments.Aug 29 2017, 11:08 AM
lib/Target/PowerPC/PPCMIPeephole.cpp
428 ↗(On Diff #111152)

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.

jtony updated this revision to Diff 113251.Aug 30 2017, 6:25 AM

Refactor the implementation according to Nemanja's suggestion (get rid of the large lambda function replaceLiWithAddi, and inline replaceAddWithCopy)

jtony marked an inline comment as done.Aug 30 2017, 6:26 AM
inouehrs added inline comments.Sep 6 2017, 6:51 AM
lib/Target/PowerPC/PPCMIPeephole.cpp
96 ↗(On Diff #113251)

It's better to make this local helper method static.

366 ↗(On Diff #113251)

Why specific to add?
Don't we have much optimization opportunity for arithmetic operations other than add?

375 ↗(On Diff #113251)
return DefPhiMI && (DefPhiMI->getOpcode() != PPC::PHI &&
       MRI->hasOneNonDBGUse(DefPhiMI->getOperand(0).getReg());

might be clearer (but its up to your preference.)

407 ↗(On Diff #113251)

We don't have and ADD fed -> redundant "and"?

409 ↗(On Diff #113251)

Now we now -> do you mean "Now we know"?

jtony marked 5 inline comments as done.Sep 7 2017, 11:44 AM
jtony added inline comments.
lib/Target/PowerPC/PPCMIPeephole.cpp
366 ↗(On Diff #113251)

Discussed in our scrum, we don't want to do this for now.

375 ↗(On Diff #113251)

That's indeed a good idea, I will change it. But the code you provided has a small error in it. It should be DefPhiMI->getOpcode() == PPC::PHI, i.e.

return DefPhiMI && (DefPhiMI->getOpcode() == PPC::PHI &&

MRI->hasOneNonDBGUse(DefPhiMI->getOperand(0).getReg());
jtony updated this revision to Diff 114282.Sep 7 2017, 4:34 PM
jtony marked 2 inline comments as done.

Address comments from Hiroshi and rebase on trunk.

jtony marked 2 inline comments as done.Sep 7 2017, 4:34 PM
nemanjai accepted this revision.Sep 18 2017, 5:45 AM

LGTM. Just a request for a comment to be added to the code.

lib/Target/PowerPC/PPCMIPeephole.cpp
366 ↗(On Diff #113251)

It is fine to leave it as is for the time being. But please add a comment such as this:

// FIXME: Any instruction that has an immediate form fed only by
// a PHI whose operands are all load immediate can be folded away.
// We currently do this for ADD instructions, but should expand it to
// arithmetic and binary instructions with immediate forms in the future.
This revision is now accepted and ready to land.Sep 18 2017, 5:45 AM
This revision was automatically updated to reflect the committed changes.