Diff Detail
Event Timeline
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.
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... |
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). |
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. |
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. |
Some minor changes based on comments. Also separated the model from the code for adding PostRA passes. Will post a separate patch for that.
Few inline comments, nothing major and nothing that can't be added later.
lib/Target/PowerPC/P9InstrResources.td | ||
---|---|---|
18 | 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 :) | |
220 | s/was/is and add a TODO: in front. |
lib/Target/PowerPC/P9InstrResources.td | ||
---|---|---|
18 | I agree :) We have gone over this in a couple of meetings to ensure it is reasonable. I will add some comments. |
I expect this file should have a license header, similar to the other files.