This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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 https://reviews.llvm.org/D46377 and the author @nemanjai found out that patch can use some existing logic in later committed patch https://reviews.llvm.org/D49007.

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
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2569

What about the IdxOfADDI and IdxOfLI ?

2573

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

2587

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

2598

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)
         break;
   }

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

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

3874

MI must have x-form opcode.

3904

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.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2653

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.

3904

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
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3904

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.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3904

agree

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

LGTM now with one minor nit.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3906

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.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3906

Will update in the commit patch

This revision was automatically updated to reflect the committed changes.