Page MenuHomePhabricator

[Power9] Processor Model for Scheduling
ClosedPublic

Authored by amehsan on Sep 13 2016, 3:11 PM.

Diff Detail

Event Timeline

amehsan updated this revision to Diff 71245.Sep 13 2016, 3:11 PM
amehsan retitled this revision from to [Power9] Processor Model for Scheduling.
amehsan updated this object.
amehsan added reviewers: hfinkel, kbarton, nemanjai.
amehsan added a subscriber: llvm-commits.

Some parameters (for example those defined in SchedMachineModel in the beginning of PPCScheduleP9.td) are subject to change after more extensive test which is scheduled for a later date.

echristo edited edge metadata.Sep 20 2016, 11:49 AM

More names for the functional units etc, more comments in general.

One important inline comment.

-eric

lib/Target/PowerPC/P9InstrResources.td
208

No commented out lines without... well, comments.

lib/Target/PowerPC/PPCTargetMachine.cpp
302 ↗(On Diff #71245)

No, this won't work. The subtarget can change on a function by function basis so you'll need to come up with an alternate way to do this.

Let's see what we can come up with...

amehsan added inline comments.Sep 21 2016, 6:39 PM
lib/Target/PowerPC/PPCTargetMachine.cpp
302 ↗(On Diff #71245)

I did not know that subtarget object owned by PPCTargetMachine will be removed. Anyway, I am thinking of a solution along this lines (not entirely verified it though):

First we create a hook for adding PostRA sched pass. Then for PPC we add both PostRA schedulers to the pipeline. In the beginning of the run method for each pass, we add a target specific hook. For PPC we check target-cpu of the machine function. Does this sounds reasonable? (I still need to verify that P8Model will be available for functions with target-cpu=pwr8 when a module is compile with -mcpu=pwr9).

kbarton requested changes to this revision.Sep 22 2016, 12:40 PM
kbarton edited edge metadata.
kbarton added inline comments.
lib/Target/PowerPC/P9InstrResources.td
2

I expect this file should have a license header, similar to the other files.

208

Yes, I agree. Any instructions commented out should have a comment explaining why.

lib/Target/PowerPC/PPCScheduleP9.td
11

Fix comment.

lib/Target/PowerPC/PPCTargetMachine.cpp
302 ↗(On Diff #71245)

This sounds reasonable to me, but I'm not overly familiar with how the functionality is tied together at the moment.

Is there not already a hook for adding a PostRA sched pass?

lib/Target/PowerPC/PPCTargetMachine.h
60 ↗(On Diff #71245)

I don't know why exactly, but this check looks really suspicious to me. I thought there was a different (better) way to check for the CPU. That said, I imagine this will change based on Eric's comments below.

This revision now requires changes to proceed.Sep 22 2016, 12:40 PM
amehsan added inline comments.Sep 22 2016, 12:48 PM
lib/Target/PowerPC/PPCTargetMachine.cpp
302 ↗(On Diff #71245)

PostRA sched is added in TargetPassConfig::addMachinePasses(). There is a comment where it is declared:

/// Fully developed targets will not generally override this.
virtual void addMachinePasses();

and we do not override it. It makes sense given the structure of the code. All we need to do is to add a new function to the class, specifically for adding PostRA sched and override it. It is straightforward.

I will break this patch to two parts. One that includes the model. And another one to turn on the MIScheduler for PWR9. I will use the approach that I mentioned in the comments for that one. I discussed it with Hal last week and the approach sounds reasonable.

lib/Target/PowerPC/P9InstrResources.td
208

Apparently they were inadvertently omitted when we added support for the instruction in the new ISA. Will uncomment these after we add support for them.

amehsan updated this revision to Diff 73517.Oct 4 2016, 11:37 AM
amehsan edited edge metadata.

Some minor changes based on comments. Also separated the model from the code for adding PostRA passes. Will post a separate patch for that.

amehsan updated this revision to Diff 73522.Oct 4 2016, 11:40 AM
amehsan edited edge metadata.

I added a comment to the td file, regarding the commented insns

kbarton accepted this revision.Oct 20 2016, 12:10 PM
kbarton edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 20 2016, 12:10 PM
echristo accepted this revision.Oct 20 2016, 12:15 PM
echristo edited edge metadata.

Few inline comments, nothing major and nothing that can't be added later.

lib/Target/PowerPC/P9InstrResources.td
17

General request: It would be nice if the pipeline were documented in some way here. Otherwise this is pretty much a wall of incomprehensible text :)

219

s/was/is

and add a TODO: in front.

amehsan added inline comments.Oct 20 2016, 1:33 PM
lib/Target/PowerPC/P9InstrResources.td
17

I agree :) We have gone over this in a couple of meetings to ensure it is reasonable. I will add some comments.