Page MenuHomePhabricator

[Power9] Fix the missing pseudo instruction scheduling information for power9
Needs ReviewPublic

Authored by steven.zhang on Sep 18 2019, 12:05 AM.

Details

Reviewers
jsji
nemanjai
kbarton
hfinkel
Group Reviewers
Restricted Project
Summary

For now, we didn't set any scheduling information for pseudo instructions. However, many pseudo instruction(i.e. LDtocL etc) should have the scheduling information. Create a new class for those pseudo instruction that require the scheduling information to improve the scheduler.

Diff Detail

Event Timeline

steven.zhang created this revision.Sep 18 2019, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 12:05 AM
jsji added a reviewer: Restricted Project.Sep 18 2019, 7:31 AM
jsji requested changes to this revision.Oct 2 2019, 10:52 AM

This basically LGTM.

However, I'd like us to add more tests before we land it.

The change might not change the final schedule for existing codegen testcases ,
but the information during scheduling should change,
so I think it shouldn't be too hard to come up with a few MIR tests that show the impact,
especially for toc related Pseudoes and floating point load/store psedudoes (eg: DFSTOREf32, DFLOADf32).

llvm/lib/Target/PowerPC/PPCInstrFormats.td
2195

We did have SchedInfo in P9InstrResources.td before, so are they ignored? and always using the default latency?

This revision now requires changes to proceed.Oct 2 2019, 10:52 AM
steven.zhang planned changes to this revision.Oct 20 2019, 8:02 PM
steven.zhang marked an inline comment as done.
steven.zhang set the repository for this revision to rG LLVM Github Monorepo.

Rebase the patch and add one test as Jinsong request.

If we didn't clear the hasNoSchedulingInfo for instructions, it didn't need to provide the scheduling information even we mark the model as complete. And that will result in some instructions scheduling info missing. I.e. for the pseudo load(LDtocL), it will use the default latency specified in the SchedModel, that is 5 cycles. And now, it is correct to 4 cycles.

llvm/lib/Target/PowerPC/PPCInstrFormats.td
2195

No, it will still use the latency that we specify in the resources.td. The bit here affect the checker.

I think this is a good change. My only question is with the test case. If we can get the necessary information out of MIR, then I think it's better to write an MIR test case instead of relying on the debug output from the machine scheduler.

llvm/test/CodeGen/PowerPC/pseudo-inst-latency.ll
2

Could this be done as an MIR test instead of relying on the debug output from the machine scheduler?

MaskRay added inline comments.Mar 2 2020, 9:18 PM
llvm/test/CodeGen/PowerPC/pseudo-inst-latency.ll
9

Space after colon