Page MenuHomePhabricator

[Scheduling] Add a mutation to schedule GOT indirect instructions close to each other for linker optimization
Needs ReviewPublic

Authored by steven.zhang on Sep 2 2020, 10:26 PM.

Details

Reviewers
nemanjai
stefanp
jsji
qiucf
Group Reviewers
Restricted Project
Summary

The linker optimization for pcrel load/store compiler side change has been added by D79864. However, we still failed to perform this optimization for some simple case:

pld 3, m@got@pcrel(0), 1
li 4, 4
stw 4, 0(3)
blr

And this is what linker want to do:

10010600:	02 00 10 06 	pstw    r4,132008
10010604:	04 00 80 38 	li      r4,4
10010608:	a8 03 80 90
1001060c:	00 00 00 60 	nop
10010610:	20 00 80 4e 	blr

The li 4, 4 here disable the optimization as the above transformation is wrong because we are define the r4 in betwwen pld and stw. So, we want to schedule such kind of instructions before the pld to avoid this happens.

Diff Detail

Unit TestsFailed

TimeTest
60 mslinux > LLVM.CodeGen/PowerPC::pcrel-tail-calls.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll
60 mswindows > LLVM.CodeGen/PowerPC::pcrel-tail-calls.ll
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\pcrel-tail-calls.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\pcrel-tail-calls.ll

Event Timeline

steven.zhang created this revision.Sep 2 2020, 10:26 PM
steven.zhang requested review of this revision.Sep 2 2020, 10:26 PM
steven.zhang added inline comments.
llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll
24

Is it a bug of our peephole that miss to create the relocation symbol ?

Update tests.

Gentle ping...

The fact that we catch all these additional opportunities is great. I don't fully understand all the code in the patch because I am not all that familiar with the scheduler so I'd like @stefanp to have a look at this as well.
I do have some nits inline and I'd like to see some tests that exercise the code paths where we can't reorder instructions to allow the optimization (i.e. cases where we return nullptr from getPLDpcAndLdStPair() as well as cases where we are unable to add the edges in schedHazardInsts()).

llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp
165

It seems to me like this function could be documented a bit better to explain not only what we are doing but why. Of course, this may just be because I am not familiar with the scheduler so the code isn't self-documenting to me.

276

Sorry, I don't think this comment is clear enough. What is redefined? The concept of redefinition seems at odds with SSA.

292

This appears to make the assumption that operand 2 is the base address register. The comment should probably mention that.

Simplify the logic and add tests. We still have cases that Peephole cannot handle due to missing the dependency information. But we don't want to handle it inside the scheduler as that is not right.

steven.zhang added inline comments.Sep 13 2020, 6:32 PM
llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll
26

std and pstd is not aliased. We can still do the linker opt for it but now, we can't as peephole didn't know they are not aliased. Further, we didn't emit the symbol because we have uses in-between pld and std which is too conservative also. If the other instr is store, we won't have problems if there is any uses in-between the pair.

Gentle ping ...

Overall I think that the patch looks good. I just had a couple of comments.
Thank you for finding the issues related to the peephole!

llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp
192

Do we need to check that there isn't already a dependency between Pred and PLDpc?

If PLDpc already depends on Pred then there is nothing more to do and you probably don't need the artificial edge.
If Pred depends on PLDpc then we cannot add a new dependency because that would create a circular dependency.

198

I agree. We do need to merge the functionality of hasPCRelForm from here and hasPCRelativeForm into one place. (In a different patch though...)

272

nit:
Assume

llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll
26

Yes there is room for improvement in the peephole because we do not check aliases at all. This could potentially be a bug if we clobber memory without knowing it. Good catch!
The peephole also does not differentiate between loads and stores. I agree it is safe to have uses if the instruction is a store. However, it is not safe to have uses if the second instruction is a load.

steven.zhang added inline comments.Wed, Sep 23, 4:47 PM
llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp
192

Do we need to check that there isn't already a dependency between Pred and PLDpc?

Yes

If PLDpc already depends on Pred then there is nothing more to do and you probably don't need the artificial edge.

We need to check all the preds of PLDpc which will take up some time. And I think, there is not big problem to have an artificial edge here.

If Pred depends on PLDpc then we cannot add a new dependency because that would create a circular dependency.

It has been already checked by addEdge.

198

Agree

llvm/test/CodeGen/PowerPC/pcrel-tail-calls.ll
26

Yes, we need some follow up patch to fix this issue as such kind of bug is really hard to address.

Address comments.