[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. #82634

michaelmaitland · 2024-02-22T15:35:32Z

All SchedModel files have a line that looks like:

def SomeModel : SchedMachineModel;
let SchedModel = SomeModel in {
  ...
}

TableGen requires that all records defined within the top level let must have a field SchedModel somewhere in their nested record hierarchy (i.e. the record has a field SchedModel : SchedMachineModel or recursively, one of its members has a field SchedModel : SchedMachineModel).

Classes such as SchedPredicate have added a field SchedModel : SchedMachineModel, even though the field is never used, just to supress errors (not warnings) caused from having the top level let in the model files. This decision was made to avoid having hundreds of the same let statement littered in every scheduler model file.

The reason we have never seen an error for SchedVar before is because SchedVar is never instantiated with a def. Instead, it is only created as a value that is consumed by SchedWriteVariant:

... : SchedWriteVariant<[SchedVar<...>, SchedVar<...>]>;

There is a problem with this style of instantiation. In particular, the problem arises as we try to take a class based approach to building scheduler models. I will describe the problem from the bottom up.

The LMULWriteResMXVariant multiclass takes in a SchedPredicateBase Pred. Today, the RISCVSchedSiFive7.td file defines VLDSX0Pred outside the scope of any class. That means that VLDSX0Pred exists before LMULWriteResMXVariant multiclass is instantiated. With this approach, there is no error since the predicate is instantated in entirety before the variant multiclass is instantiated. However, I have the intention to move the definition of both the predicate and the variant multiclass records inside a multiclass to factor out common parts between multiple scheduler models.

I plan to have something like:

multiclass SiFive7Base<SiFive7BaseConfig c> {
  def VLDSX0Pred : ...;
  // Need defvar since record is prefixed with NAME.
  defvar VLDSX0Pred = !cast<...>(NAME # VLDSX0Pred);
  defm SiFive7 : LMULWriteResMXVariant<VLDSX0Pred>;
}

defm "SiFive7Version1" : SiFive7Base<SiFive7BaseConfig<...>>;
defm "SiFive7Version2" : SiFive7Base<SiFive7BaseConfig<...>>;

In this scheme, VLDSX0Pred is defined within the same multiclass transaction that the LMULWriteResMXVariant is defined in. For some reason, TableGen does not allow Values to reference records that were created in the same parent record construction. If the SchedVar is not a def, then it will not be able to find the record NAME # VLDSX0Pred. Making it a def, allows TableGen to find NAME # VLDSX0Pred in scope.

The simplest example of this is:

class A {}
class B<A a> { A x = a;}
class C<B b> { B y = b;}
multiclass D {
  def MyA : A;
  defvar aa = !cast<A>(NAME # MyA);
  // This works
  def : B<aa>;
  // This does not work because constructing B by value cannot find `NAME # MyA`
  // error: Undefined reference to record: 'MyA'
  def : C<B<aa>>;
  // To fix it, define it like such:
  def MyB : B<aa>;
  defvar bb = !cast<B>(NAME # MyB);
  def : C<bb>;
}
defm "" : D;

In summary, in order to use a class based approach to creating scheduler resources to promote resusability, SchedVars must be created using defs instead of being instantiated by value so that it can resolve records that were part of the instantiation of the parent record being created. In order to do this without refactoring the top level let statement that all scheduler model files use, we add an unused field SchedModel : SchedMachineModel to SchedVar, similiar to what has been done in SchedPredicate.

…les. All SchedModel files have a line that looks like: ``` def SomeModel : SchedMachineModel; let SchedModel = SomeModel in { ... } ``` TableGen requires that all records defined within the top level `let` must have a field `SchedModel` somewhere in their nested record hierarchy (i.e. the record has a field `SchedModel : SchedMachineModel` or recursively, one of its members has a field `SchedModel : SchedMachineModel`. Classes such as `SchedPredicate` have added a field `SchedModel : SchedMachineModel`, even though the field is never used, just to supress **errors** (not warnings) caused from having the top level let in the model files. This decision was made to avoid having hundreds of the same `let` statement littered in every scheduler model file. The reason we have never seen an error for `SchedVar` before is because `SchedVar` is never instantiated with a `def`. Instead, it is only created as a value that is consumed by `SchedWriteVariant`: ``` ... : SchedWriteVariant<[SchedVar<...>, SchedVar<...>]>; ``` There is a problem with this style of instantiation. In particular, the problem arises as we try to take a class based approach to building scheduler models. I will describe the problem from the bottom up. The `LMULWriteResMXVariant` multiclass takes in a `SchedPredicateBase Pred`. Today, the RISCVSchedSiFive7.td file defines `VLDSX0Pred` outside the scope of any class. That means that `VLDSX0Pred` exists before `LMULWriteResMXVariant` multiclass is instantiated. With this approach, there is no error since the predicate is instantated in entirety before the variant multiclass is instantiated. However, I have the intention to move the definition of both the predicate and the variant multiclass records inside a multiclass to factor out common parts between multiple scheduler models. I plan to have something like: ``` multiclass SiFive7Base<SiFive7BaseConfig c> { def VLDSX0Pred : ...; // Need defvar since record is prefixed with NAME. defvar VLDSX0Pred = !cast<...>(NAME # VLDSX0Pred); defm SiFive7 : LMULWriteResMXVariant<VLDSX0Pred>; } defm "" :SiFive7Version1<SiFive7BaseConfig<...>>; defm "" :SiFive7Version2<SiFive7BaseConfig<...>>; ``` In this scheme, VLDSX0Pred is defined within the same multiclass transaction that the `LMULWriteResMXVariant` is defined in. For some reason, TableGen does not allow `Values` to reference records that were created in the same parent record construction. If the `SchedVar` is not a `def`, then it will not be able to find the record `NAME # VLDSX0Pred`. Making it a def, allows TableGen to find `NAME # VLDSX0Pred` in scope. The simplest example of this is: ``` class A {} class B<A a> { A x = a;} class C<B b> { B y = b;} multiclass D { def MyA : A; defvar aa = !cast<A>(NAME # MyA); // This works def : B<aa>; // This does not work because constructing B by value cannot find `NAME # MyA` // error: Undefined reference to record: 'MyA' def : C<B<aa>>; // To fix it, define it like such: def MyB : B<aa>; defvar bb = !cast<B>(NAME # MyB); def : C<bb>; } defm "" : D; ``` In summary, in order to use a class based approach to creating scheduler resources to promote resusability, `SchedVar`s must be created using defs instead of being instantiated by value so that it can resolve records that were part of the instantiation of the parent record being created. In order to do this without refactoring the top level `let` statement that all scheduler model files use, we add an unused field `SchedModel : SchedMachineModel` to `SchedVar`, similiar to what has been done in `SchedPredicate`.

llvmbot · 2024-02-22T15:35:51Z

@llvm/pr-subscribers-backend-risc-v

Author: Michael Maitland (michaelmaitland)

Changes

…les.

All SchedModel files have a line that looks like:

def SomeModel : SchedMachineModel;
let SchedModel = SomeModel in {
  ...
}

TableGen requires that all records defined within the top level let must have a field SchedModel somewhere in their nested record hierarchy (i.e. the record has a field SchedModel : SchedMachineModel or recursively, one of its members has a field SchedModel : SchedMachineModel.

Classes such as SchedPredicate have added a field SchedModel : SchedMachineModel, even though the field is never used, just to supress errors (not warnings) caused from having the top level let in the model files. This decision was made to avoid having hundreds of the same let statement littered in every scheduler model file.

The reason we have never seen an error for SchedVar before is because SchedVar is never instantiated with a def. Instead, it is only created as a value that is consumed by SchedWriteVariant:

... : SchedWriteVariant&lt;[SchedVar&lt;...&gt;, SchedVar&lt;...&gt;]&gt;;

There is a problem with this style of instantiation. In particular, the problem arises as we try to take a class based approach to building scheduler models. I will describe the problem from the bottom up.

The LMULWriteResMXVariant multiclass takes in a SchedPredicateBase Pred. Today, the RISCVSchedSiFive7.td file defines VLDSX0Pred outside the scope of any class. That means that VLDSX0Pred exists before LMULWriteResMXVariant multiclass is instantiated. With this approach, there is no error since the predicate is instantated in entirety before the variant multiclass is instantiated. However, I have the intention to move the definition of both the predicate and the variant multiclass records inside a multiclass to factor out common parts between multiple scheduler models.

I plan to have something like:

multiclass SiFive7Base&lt;SiFive7BaseConfig c&gt; {
  def VLDSX0Pred : ...;
  // Need defvar since record is prefixed with NAME.
  defvar VLDSX0Pred = !cast&lt;...&gt;(NAME # VLDSX0Pred);
  defm SiFive7 : LMULWriteResMXVariant&lt;VLDSX0Pred&gt;;
}

defm "" :SiFive7Version1&lt;SiFive7BaseConfig&lt;...&gt;&gt;;
defm "" :SiFive7Version2&lt;SiFive7BaseConfig&lt;...&gt;&gt;;

In this scheme, VLDSX0Pred is defined within the same multiclass transaction that the LMULWriteResMXVariant is defined in. For some reason, TableGen does not allow Values to reference records that were created in the same parent record construction. If the SchedVar is not a def, then it will not be able to find the record NAME # VLDSX0Pred. Making it a def, allows TableGen to find NAME # VLDSX0Pred in scope.

The simplest example of this is:

class A {}
class B&lt;A a&gt; { A x = a;}
class C&lt;B b&gt; { B y = b;}
multiclass D {
  def MyA : A;
  defvar aa = !cast&lt;A&gt;(NAME # MyA);
  // This works
  def : B&lt;aa&gt;;
  // This does not work because constructing B by value cannot find `NAME # MyA`
  // error: Undefined reference to record: 'MyA'
  def : C&lt;B&lt;aa&gt;&gt;;
  // To fix it, define it like such:
  def MyB : B&lt;aa&gt;;
  defvar bb = !cast&lt;B&gt;(NAME # MyB);
  def : C&lt;bb&gt;;
}
defm "" : D;

In summary, in order to use a class based approach to creating scheduler resources to promote resusability, SchedVars must be created using defs instead of being instantiated by value so that it can resolve records that were part of the instantiation of the parent record being created. In order to do this without refactoring the top level let statement that all scheduler model files use, we add an unused field SchedModel : SchedMachineModel to SchedVar, similiar to what has been done in SchedPredicate.

Full diff: https://github.com/llvm/llvm-project/pull/82634.diff

2 Files Affected:

(modified) llvm/include/llvm/Target/TargetSchedule.td (+2)
(modified) llvm/lib/Target/RISCV/RISCVScheduleV.td (+13-8)

diff --git a/llvm/include/llvm/Target/TargetSchedule.td b/llvm/include/llvm/Target/TargetSchedule.td
index e2781a5d1ea54a..48c9387977c075 100644
--- a/llvm/include/llvm/Target/TargetSchedule.td
+++ b/llvm/include/llvm/Target/TargetSchedule.td
@@ -399,6 +399,8 @@ def NoSchedPred : MCSchedPredicate<TruePred>;
 class SchedVar<SchedPredicateBase pred, list<SchedReadWrite> selected> {
   SchedPredicateBase Predicate = pred;
   list<SchedReadWrite> Selected = selected;
+  // SchedModel silences warnings but is ignored.
+  SchedMachineModel SchedModel = ?;
 }
 
 // SchedModel silences warnings but is ignored.
diff --git a/llvm/lib/Target/RISCV/RISCVScheduleV.td b/llvm/lib/Target/RISCV/RISCVScheduleV.td
index d15cb611ae665e..0be681de3daf69 100644
--- a/llvm/lib/Target/RISCV/RISCVScheduleV.td
+++ b/llvm/lib/Target/RISCV/RISCVScheduleV.td
@@ -88,20 +88,25 @@ multiclass LMULWriteResMXVariant<string name, SchedPredicateBase Pred,
     let ReleaseAtCycles = noPredReleaseCycles;
   }
 
+  // Define SchedVars
+  def nameMX # PredSchedVar
+      : SchedVar<Pred, [!cast<SchedWriteRes>(NAME # nameMX # "_Pred")]>;
+  def nameMX # NoPredSchedVar
+      : SchedVar<NoSchedPred, [!cast<SchedWriteRes>(NAME # nameMX #"_NoPred")]>;
+  // Allow multiclass to refer to SchedVars -- need to have NAME prefix.
+  defvar PredSchedVar = !cast<SchedVar>(NAME # nameMX # PredSchedVar);
+  defvar NoPredSchedVar = !cast<SchedVar>(NAME # nameMX # NoPredSchedVar);
+
   // Tie behavior to predicate
-  def NAME # nameMX # "_Variant" : SchedWriteVariant<[
-    SchedVar<Pred, [!cast<SchedWriteRes>(NAME # nameMX # "_Pred")]>,
-    SchedVar<NoSchedPred, [!cast<SchedWriteRes>(NAME # nameMX # "_NoPred")]>
-  ]>;
+  def NAME # nameMX # "_Variant"
+      : SchedWriteVariant<[PredSchedVar, NoPredSchedVar]>;
   def : SchedAlias<
     !cast<SchedReadWrite>(nameMX),
     !cast<SchedReadWrite>(NAME # nameMX # "_Variant")>;
 
   if IsWorstCase then {
-    def NAME # name # "_WorstCase_Variant" : SchedWriteVariant<[
-      SchedVar<Pred, [!cast<SchedWriteRes>(NAME # nameMX # "_Pred")]>,
-      SchedVar<NoSchedPred, [!cast<SchedWriteRes>(NAME # nameMX # "_NoPred")]>
-    ]>;
+    def NAME # name # "_WorstCase_Variant"
+      : SchedWriteVariant<[PredSchedVar, NoPredSchedVar]>;
     def : SchedAlias<
       !cast<SchedReadWrite>(name # "_WorstCase"),
       !cast<SchedReadWrite>(NAME # name # "_WorstCase_Variant")>;

topperc · 2024-02-22T20:18:36Z

Why does VLDSX0Pred need to be inside the SiFive7Base class? There's nothing that changes based on which model uses it. It could just be a top level record in RISCVScheduleV.td. ARM, AArch64, and X86 have a SchedPredicates.td file. I don't think we have enough predicates yet to justify that, but my point is that sched predicates can be independent of the model.

michaelmaitland · 2024-02-22T20:39:59Z

Why does VLDSX0Pred need to be inside the SiFive7Base class? There's nothing that changes based on which model uses it. It could just be a top level record in RISCVScheduleV.td. ARM, AArch64, and X86 have a SchedPredicates.td file. I don't think we have enough predicates yet to justify that, but my point is that sched predicates can be independent of the model.

SchedPredicate class has access to the SchedModel. For example, it would be okay to say:

def MyPred : SchedPredicate<[{SchedModel->getSubtargetInfo()->hasFeature(RISCV::TuneMyFeature)}]>;

In these cases, the predicate needs to be defined under the let statement.

topperc · 2024-02-22T20:43:03Z

Why does VLDSX0Pred need to be inside the SiFive7Base class? There's nothing that changes based on which model uses it. It could just be a top level record in RISCVScheduleV.td. ARM, AArch64, and X86 have a SchedPredicates.td file. I don't think we have enough predicates yet to justify that, but my point is that sched predicates can be independent of the model.

SchedPredicate class has access to the SchedModel. For example, it would be okay to say:
def MyPred : SchedPredicate<[{SchedModel->getSubtargetInfo()->hasFeature(RISCV::TuneMyFeature)}]>;
In these cases, the predicate needs to be defined under the let statement.

But we're talking about an MCSchedPredicate here not a SchedPredicate. Does an MCSchedPredicate have access to the model?

topperc · 2024-02-22T20:45:07Z

Why does VLDSX0Pred need to be inside the SiFive7Base class? There's nothing that changes based on which model uses it. It could just be a top level record in RISCVScheduleV.td. ARM, AArch64, and X86 have a SchedPredicates.td file. I don't think we have enough predicates yet to justify that, but my point is that sched predicates can be independent of the model.

SchedPredicate class has access to the SchedModel. For example, it would be okay to say:
def MyPred : SchedPredicate<[{SchedModel->getSubtargetInfo()->hasFeature(RISCV::TuneMyFeature)}]>;
In these cases, the predicate needs to be defined under the let statement.

Why? That SchedModel is coming from C++ code not from tablegen. It does not involve the let statement at all. It's the scheduler model that is active at the time the predicate is called. If the active scheduler model doesn't make reference to the predicate it won't ever be called when that scheduler model is active.

michaelmaitland · 2024-02-22T21:01:05Z

We can move the predicate out, that is unrelated to this patch. We still need to def the SchedVar, otherwise it wont be able to find the SchedReadWrite that was defined just a few lines above.

topperc

LGTM

wangpc-pp

LGTM.

For some reason, TableGen does not allow Values to reference records that were created in the same parent record construction.

Yeah, this is a problem. I may make TableGen able to refer to other definitions in a multiclass:

multiclass Test {
  def a;
  def b: B<a>; // For `a`, we will search `NAME # a` first and then `a`.
}

(Hopefully this can be easy to design and implement)

michaelmaitland · 2024-02-23T14:18:09Z

Yeah, this is a problem. I may make TableGen able to refer to other definitions in a multiclass:
multiclass Test {
  def a;
  def b: B<a>; // For `a`, we will search `NAME # a` first and then `a`.
}
(Hopefully this can be easy to design and implement)

This would be very nice. It is quite ugly to have to use !cast everywhere for defs. Some careful though will need to be given to what happens when there is a record a and when there is a different record NAME # a -- we would need a way to refer to both. Please include me on the review if you do this work :)

wangpc-pp · 2024-03-01T10:55:57Z

Yeah, this is a problem. I may make TableGen able to refer to other definitions in a multiclass:
multiclass Test {
  def a;
  def b: B<a>; // For `a`, we will search `NAME # a` first and then `a`.
}
(Hopefully this can be easy to design and implement)
This would be very nice. It is quite ugly to have to use !cast everywhere for defs. Some careful though will need to be given to what happens when there is a record a and when there is a different record NAME # a -- we would need a way to refer to both. Please include me on the review if you do this work :)

I have tried it, it may be hard to implement based on my rough investigation. In short, the TableGen parser resolves Record after resolving variables/arguements. I need more time to think about it.

michaelmaitland added the backend:RISC-V label Feb 22, 2024

michaelmaitland requested review from preames, topperc and wangpc-pp February 22, 2024 15:35

michaelmaitland requested a review from mshockwave February 22, 2024 15:36

dtcxzyw changed the title ~~[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model fi…~~ [RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. Feb 22, 2024

michaelmaitland requested a review from dtcxzyw February 22, 2024 15:52

topperc approved these changes Feb 22, 2024

View reviewed changes

wangpc-pp approved these changes Feb 23, 2024

View reviewed changes

michaelmaitland merged commit be083db into llvm:main Feb 23, 2024
6 checks passed

michaelmaitland deleted the sched-var branch February 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. #82634

[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. #82634

michaelmaitland commented Feb 22, 2024 •

edited

llvmbot commented Feb 22, 2024

topperc commented Feb 22, 2024 •

edited

michaelmaitland commented Feb 22, 2024

topperc commented Feb 22, 2024

topperc commented Feb 22, 2024 •

edited

michaelmaitland commented Feb 22, 2024

topperc left a comment

wangpc-pp left a comment

michaelmaitland commented Feb 23, 2024

wangpc-pp commented Mar 1, 2024

[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. #82634

[RISCV][NFC] Allow SchedVar to be a def inside our scheduler model files. #82634

Conversation

michaelmaitland commented Feb 22, 2024 • edited

llvmbot commented Feb 22, 2024

topperc commented Feb 22, 2024 • edited

michaelmaitland commented Feb 22, 2024

topperc commented Feb 22, 2024

topperc commented Feb 22, 2024 • edited

michaelmaitland commented Feb 22, 2024

topperc left a comment

Choose a reason for hiding this comment

wangpc-pp left a comment

Choose a reason for hiding this comment

michaelmaitland commented Feb 23, 2024

wangpc-pp commented Mar 1, 2024

michaelmaitland commented Feb 22, 2024 •

edited

topperc commented Feb 22, 2024 •

edited

topperc commented Feb 22, 2024 •

edited