Page MenuHomePhabricator

[PowerPC] fold addi's imm operand to its imm form consumer's displacement

Authored by shchenz on Jun 12 2020, 2:15 AM.



After MachineSink, there will be peephole opportunity for:

%0:g8rc_and_g8rc_nox0 = ADDI8 %5:g8rc_and_g8rc_nox0, 144
STD killed %7:g8rc, 16, %0:g8rc_and_g8rc_nox0 :: (store 8 into %ir.8)


STD killed %7:g8rc, 160, %5:g8rc_and_g8rc_nox0 :: (store 8 into %ir.8)

This folding opportunity may become unfoldable after RA because we may redefine addi's register operand in RA. So we add this peephole before RA in convertToImmediateForm.

First try was implemented in and the author @nemanjai found out that patch can use some existing logic in later committed patch

This patch tries to do this.

Diff Detail

Event Timeline

shchenz created this revision.Jun 12 2020, 2:15 AM
shchenz edited the summary of this revision. (Show Details)Jun 12 2020, 2:18 AM
steven.zhang added inline comments.Jun 18 2020, 7:04 PM

What about the IdxOfADDI and IdxOfLI ?


Move the declaration of MITmp to line 2255. And I prefer to use some meaningful name.


Add "break" here and you don't need to check the !OpLI.


I think, you don't need two variables to keep LI and ADDI separate. How about doing it like this:

foreach operands
   if (is_li || is_addi) {
      Inst =  MITmp = MRI->getVRegDef(TrueReg);
      idx = i;
     if (is_li)

if (Inst) {
   OpNoForForwarding = idx;
   DefMI = Inst;

Do we still need to fixup the kill/dead flags if it SSA ?


MI must have x-form opcode.


We will initialize the Imm inside isImmElgibleForForwarding, and you don't need to initialize it with ImmOperandMI

shchenz updated this revision to Diff 271907.Jun 18 2020, 8:39 PM
shchenz marked 7 inline comments as done.

update according comments:
1: simplify the code logic a little bit.


Yes, we need it. kill/dead flags exist even in SSA and machineverify will use these flags to do checking.
Without these fixup, we get LIT case failure.


The Imm initialized in isImmElgibleForForwarding is just for DefMI, here we need the sum of the imm in DefMI and imm in MI.

steven.zhang added inline comments.Jun 22 2020, 4:15 PM

That’s really not a good idea to sum the imm secretly inside isImmElgibleForForwarding, as what that function does is to check if ImmMO is eligible for forwarding, if yes, extract and set the imm. Can we add another parameter named something like int64_t base which has default initializer as 0, and add some necessary comment for that function?

shchenz updated this revision to Diff 272584.Jun 22 2020, 6:29 PM
shchenz marked an inline comment as done.

address comments:
1: make the imm sum explicit in function isImmElgibleForForwarding

shchenz marked an inline comment as done.Jun 22 2020, 6:30 PM
shchenz added inline comments.


steven.zhang accepted this revision.Jun 22 2020, 7:59 PM

LGTM now with one minor nit.


Don't use the same variable to avoid confusion.

This revision is now accepted and ready to land.Jun 22 2020, 7:59 PM
shchenz marked 2 inline comments as done.Jun 23 2020, 2:25 AM

Thanks for your review.


Will update in the commit patch

This revision was automatically updated to reflect the committed changes.