This is an archive of the discontinued LLVM Phabricator instance.

[Power9] Fix the missing pseudo instruction scheduling information for power9
Changes PlannedPublic

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

Details

Reviewers
nemanjai
kbarton
hfinkel
steven.zhang
jsji
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

Are you still planning to proceed with this change? You haven't really addressed the requests for additional testing. You still have to provide the following:

  • MIR test case for some of these (or justification as to why an MIR test case won't adequately show what we are interested in)
  • Test cases for at least some more of the pseudo instructions for which you're adding scheduling info
steven.zhang planned changes to this revision.Apr 16 2020, 7:21 PM
qiucf commandeered this revision.May 27 2021, 10:51 PM
qiucf added a reviewer: steven.zhang.
jsji resigned from this revision.Jun 2 2022, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 8:00 AM