[PATCH] D24525: [Power9] Processor Model for Scheduling

[Kit Barton via llvm-commits]

kbarton requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Target/PowerPC/P9InstrResources.td:1
@@ +1,2 @@
+// Some itinerary classes include instructions with different resource
+// requirements. Resource definitions for those classes is defined at
----------------
I expect this file should have a license header, similar to the other files. 

================
Comment at: lib/Target/PowerPC/P9InstrResources.td:207
@@ +206,3 @@
+      (instrs
+ //   VABSDUB,
+ //   VABSDUH,
----------------
echristo wrote:
> No commented out lines without... well, comments.
Yes, I agree. Any instructions commented out should have a comment explaining why. 

================
Comment at: lib/Target/PowerPC/PPCScheduleP9.td:10
@@ +9,3 @@
+//
+// This file defines the itinerary class data for the POWER8 processor.
+//
----------------
Fix comment.

================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:302
@@ +301,3 @@
+    : TargetPassConfig(TM, PM) {
+    if (TM->isPWR9() && TM->getOptLevel() != CodeGenOpt::None)
+      substitutePass(&PostRASchedulerID, &PostMachineSchedulerID);
----------------
amehsan wrote:
> echristo wrote:
> > 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...
> 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).
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?

================
Comment at: lib/Target/PowerPC/PPCTargetMachine.h:60
@@ +59,3 @@
+  bool isPWR9() const {
+    return (Subtarget.getDarwinDirective() == PPC::DIR_PWR9);
+  };
----------------
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. 


https://reviews.llvm.org/D24525