Removing unsupported resources from sched model

[avt77]

Summary

This is only an idea how it's possible to resolve the issue. The patch is very small and simple and if you don't mind I could complete the one: at the momnet there are couple of failed tests with this patch applied that's why it is not the final version. But if you think I should not deal with the issue I'll simply close this review.

[courbet]

Hi Andrew,

I'm not sure I understand the principle of this patch: Would you have just one Unsupported.td file for all subtargets, or one per subtarget ?

• If the former, how are duplicate definitions handled ?
• If the latter, what's the advantage compared to inlining (as in D47763) ?

[avt77]

Obviously, the idea was to use one include file for all targets. But it did not work in the first version of the patch. Now I did a "dirty hack" in TableGen to be able to do it. It works for 2 CPUs and obviously it should work for others. The usage is very simply: the common include file should keep the common unsupported instructions while the specific onces should be inserted into the specific .td files.

There are still 2 failed sched tests but what's in general? Should I continue this effort? From my point of view it's really better than to put Unsupported flag inside every model(as in D47763).

And as I wrote it is not a real patch: I'd like only to show the idea. The "dirty hack" should be redesigned and failed tests should be fixed.

[courbet]

In D47766#1123673, @avt77 wrote:

Obviously, the idea was to use one include file for all targets. But it did not work in the first version of the patch. Now I did a "dirty hack" in TableGen to be able to do it. It works for 2 CPUs and obviously it should work for others. The usage is very simply: the common include file should keep the common unsupported instructions while the specific onces should be inserted into the specific .td files.

OK I see. So you're suggesting essentially moving all commonly unsupported instructions to a separate file.
But what's this set composed of ? instructions that are not supported by any subtarget are not really plethoric :)

[avt77]

Of course not. First of all, I suggest to put AVX2 and AVX512 instructions in separate files and to use them in models which don't support AVX2 and/or AVX512. Etc.

Should I do it? And should I cleanup the patch?

[courbet]

I see. To be honest I'm not a big fan of including files in different contexts to mean different things. A very similar but more principled approach would be to have one file to define a bunch of multiclasses:

multiclass UnsupportedFMAWriteRes {
  def : WriteRes<WriteFMA, []>;
  def : WriteRes<WriteFMA.Folded, []>;
  def : WriteRes<WriteFMAX, []>;
  def : WriteRes<WriteFMAX.Folded, []>;
  def : WriteRes<WriteFMAY, []>;
  def : WriteRes<WriteFMAY.Folded, []>;
}

Then in the model:

let SchedModel = BtVer2Model in {
//...
defm : UnsupportedFMAWriteRes;
//...
}

Then I'm not sure if we win a lot from this as it makes things less explicit, though I agree that it slightly reduces code duplication.

[avt77]

In D47766#1124565, @courbet wrote:

Then I'm not sure if we win a lot from this as it makes things less explicit, though I agree that it slightly reduces code duplication.

Simon, what do you think about? Should I prepare the real patch following corbet's suggestion? From my point of view the code like

let SchedModel = BtVer2Model in {
...
defm : UnsupportedFMAWriteRes;
...
}

is absolutely explicit. (Maybe plus comments if name is not enough.). And I should not change TableGen source code.

@corbet, let's make decision about this patch: to do or not to do?

[courbet]

In D47766#1124615, @avt77 wrote:

In D47766#1124565, @courbet wrote:

Then I'm not sure if we win a lot from this as it makes things less explicit, though I agree that it slightly reduces code duplication.

Simon, what do you think about? Should I prepare the real patch following corbet's suggestion? From my point of view the code like

let SchedModel = BtVer2Model in {
...
defm : UnsupportedFMAWriteRes;
...
}

is absolutely explicit. (Maybe plus comments if name is not enough.). And I should not change TableGen source code.

@corbet, let's make decision about this patch: to do or not to do?

No strong opinion; let's see what Simon thinks.

[RKSimon]

I really don't like the idea of a separate file - I think the models need to stay self contained. @courbet 's approach in D47763 seems a lot tidier

[avt77]

9>>! In D47766#1124708, @RKSimon wrote:

I really don't like the idea of a separate file - I think the models need to stay self contained. @courbet 's approach in D47763 seems a lot tidier

As I remember there was a discussion about a possibility to use some "common" possibility to mark unsupported/default features w/o necessity to repeat those unsupported features in all models. The idea was to put such things in some place and to "inherit" or "redefine" them. I tried to implement this behaviour in this patch: we should not repeat description of unsupported features inside every model; we should simply include some file(s) with common description of those features.

On the other hand I see now X86WriteResUnsupported/X86WriteResPairUnsupported in D47763 and I like it for unsupported resources. Because of that I'm going to close this review.

[RKSimon]

In D47766#1124959, @avt77 wrote:

9>>! In D47766#1124708, @RKSimon wrote:

I really don't like the idea of a separate file - I think the models need to stay self contained. @courbet 's approach in D47763 seems a lot tidier

As I remember there was a discussion about a possibility to use some "common" possibility to mark unsupported/default features w/o necessity to repeat those unsupported features in all models. The idea was to put such things in some place and to "inherit" or "redefine" them. I tried to implement this behaviour in this patch: we should not repeat description of unsupported features inside every model; we should simply include some file(s) with common description of those features.

On the other hand I see now X86WriteResUnsupported/X86WriteResPairUnsupported in D47763 and I like it for unsupported resources. Because of that I'm going to close this review.

You mean PR36924? Yes that was about the idea of having an inheritance of classes so a model that didn't need to split a class by size (because they always had the same cost; or it didn't support wider vectors) could avoid the extra entries. The conclusion was that we should be trying to document all classes in all models for clarity - the approach of using the Unsupported multiclasses inserted in the correct location in each model meets those objectives.

[RKSimon]

Abandon this? D47763 took this patch's idea for a UnsupportedWriteResPair multiclass but kept the unsupported classes in target's scheduler td file.

[avt77]

The main idea of this patch is implemented in D47763.

lib/Target/X86/Unsupported.td

+//=- X86ScheduleBtVer2.td - X86 BtVer2 (Jaguar) Scheduling ---*- tablegen -*-=//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines instructions which are not supported on some CPUs but should be
+// described inside the machine model
+//
+//===----------------------------------------------------------------------===//
+
+////////////////////////////////////////////////////////////////////////////////
+// AVX2 - unsupported on Jaguar and others...
+////////////////////////////////////////////////////////////////////////////////
+
+defm : UnsupportedWriteResPair<WriteFShuffle256>;
+defm : UnsupportedWriteResPair<WriteFVarShuffle256>;
+defm : UnsupportedWriteResPair<WriteShuffle256>;
+defm : UnsupportedWriteResPair< WriteVarShuffle256>;
+defm : UnsupportedWriteResPair<WriteVarVecShift>;
+defm : UnsupportedWriteResPair<WriteVarVecShiftY>;
+
+////////////////////////////////////////////////////////////////////////////////
+// FMA - unsupported on Jaguar and others...
+////////////////////////////////////////////////////////////////////////////////
+
+defm : UnsupportedWriteResPair<WriteFMA>;
+defm : UnsupportedWriteResPair<WriteFMAX>;
+defm : UnsupportedWriteResPair<WriteFMAY>;

lib/Target/X86/X86Schedule.td

multiclass X86SchedWritePair {
   // Register-Memory operation.
   def Ld : SchedWrite;
   // Register-Register operation.
   def NAME : X86FoldableSchedWrite {
     let Folded = !cast<SchedWrite>(NAME#"Ld");
   }
 }
 
+multiclass UnsupportedWriteResPair<X86FoldableSchedWrite SchedRW> {
+  def : WriteRes<SchedRW, [] > { let Unsupported = 1;  }
+  def : WriteRes<SchedRW.Folded, []> { let Unsupported = 1;  }
+}
+
 // Multiclass that wraps X86FoldableSchedWrite for each vector width.
 class X86SchedWriteWidths<X86FoldableSchedWrite sScl,
                           X86FoldableSchedWrite s128,
                           X86FoldableSchedWrite s256,
                           X86FoldableSchedWrite s512> {
   X86FoldableSchedWrite Scl = sScl; // Scalar float/double operations.
   X86FoldableSchedWrite MMX = sScl; // MMX operations.
   X86FoldableSchedWrite XMM = s128; // XMM operations.

lib/Target/X86/X86ScheduleBtVer2.td

 defm : JWriteResYMMPair<WriteFCmp64Y,      [JFPU0, JFPA],  2, [2,2], 2>;
 defm : JWriteResFpuPair<WriteFCom,  [JFPU0, JFPA, JALU0],  3>;
 defm : JWriteResFpuPair<WriteFMul,         [JFPU1, JFPM],  2>;
 defm : JWriteResFpuPair<WriteFMulX,        [JFPU1, JFPM],  2>;
 defm : JWriteResYMMPair<WriteFMulY,        [JFPU1, JFPM],  2, [2,2], 2>;
 defm : JWriteResFpuPair<WriteFMul64,       [JFPU1, JFPM],  4, [1,2]>;
 defm : JWriteResFpuPair<WriteFMul64X,      [JFPU1, JFPM],  4, [1,2]>;
 defm : JWriteResYMMPair<WriteFMul64Y,      [JFPU1, JFPM],  4, [2,4], 2>;
-defm : JWriteResFpuPair<WriteFMA,          [JFPU1, JFPM],  2>; // NOTE: Doesn't exist on Jaguar.
-defm : JWriteResFpuPair<WriteFMAX,         [JFPU1, JFPM],  2>; // NOTE: Doesn't exist on Jaguar.
-defm : JWriteResFpuPair<WriteFMAY,         [JFPU1, JFPM],  2>; // NOTE: Doesn't exist on Jaguar.
 defm : JWriteResFpuPair<WriteDPPD,   [JFPU1, JFPM, JFPA],  9, [1, 3, 3],  3>;
 defm : JWriteResFpuPair<WriteDPPS,   [JFPU1, JFPM, JFPA], 11, [1, 3, 3],  5>;
 defm : JWriteResYMMPair<WriteDPPSY,  [JFPU1, JFPM, JFPA], 12, [2, 6, 6], 10>;
 defm : JWriteResFpuPair<WriteFRcp,         [JFPU1, JFPM],  2>;
 defm : JWriteResFpuPair<WriteFRcpX,        [JFPU1, JFPM],  2>;
 defm : JWriteResYMMPair<WriteFRcpY,        [JFPU1, JFPM],  2, [2,2], 2>;
 defm : JWriteResFpuPair<WriteFRsqrt,       [JFPU1, JFPM],  2>;
 defm : JWriteResFpuPair<WriteFRsqrtX,      [JFPU1, JFPM],  2>;

 defm : JWriteResFpuPair<WriteFShuffle,    [JFPU01, JFPX],  1>;
 defm : JWriteResYMMPair<WriteFShuffleY,   [JFPU01, JFPX],  1, [2, 2], 2>;
 defm : JWriteResFpuPair<WriteFVarShuffle, [JFPU01, JFPX],  2, [1, 4], 3>;
 defm : JWriteResYMMPair<WriteFVarShuffleY,[JFPU01, JFPX],  3, [2, 6], 6>;
 defm : JWriteResFpuPair<WriteFBlend,      [JFPU01, JFPX],  1>;
 defm : JWriteResYMMPair<WriteFBlendY,     [JFPU01, JFPX],  1, [2, 2], 2>;
 defm : JWriteResFpuPair<WriteFVarBlend,   [JFPU01, JFPX],  2, [1, 4], 3>;
 defm : JWriteResYMMPair<WriteFVarBlendY,  [JFPU01, JFPX],  3, [2, 6], 6>;
-defm : JWriteResFpuPair<WriteFShuffle256, [JFPU01, JFPX],  1>;
-defm : JWriteResFpuPair<WriteFVarShuffle256, [JFPU01, JFPX],  1>; // NOTE: Doesn't exist on Jaguar.
 
 ////////////////////////////////////////////////////////////////////////////////
 // Conversions.
 ////////////////////////////////////////////////////////////////////////////////
 
 defm : JWriteResFpuPair<WriteCvtSS2I,      [JFPU1, JSTC, JFPA, JALU0], 7, [1,1], 2>;
 defm : JWriteResFpuPair<WriteCvtPS2I,      [JFPU1, JSTC], 3, [1,1], 1>;
 defm : JWriteResYMMPair<WriteCvtPS2IY,     [JFPU1, JSTC], 3, [2,2], 2>;

 defm : JWriteResFpuPair<WriteBlendY,      [JFPU01, JVALU], 1>;
 defm : JWriteResFpuPair<WriteVarBlend,    [JFPU01, JVALU], 2, [1, 4], 3>;
 defm : JWriteResFpuPair<WriteVarBlendY,   [JFPU01, JVALU], 2, [1, 4], 3>;
 defm : JWriteResFpuPair<WriteVecLogic,    [JFPU01, JVALU], 1>;
 defm : JWriteResFpuPair<WriteVecLogicX,   [JFPU01, JVALU], 1>;
 defm : JWriteResFpuPair<WriteVecLogicY,   [JFPU01, JVALU], 1>; // NOTE: Doesn't exist on Jaguar.
 defm : JWriteResFpuPair<WriteVecTest,     [JFPU0, JFPA, JALU0], 3>;
 defm : JWriteResYMMPair<WriteVecTestY ,   [JFPU01, JFPX, JFPA, JALU0], 4, [2, 2, 2, 1], 3>;
-defm : JWriteResFpuPair<WriteShuffle256,  [JFPU01, JVALU], 1>;
-defm : JWriteResFpuPair<WriteVarShuffle256, [JFPU01, JVALU], 1>; // NOTE: Doesn't exist on Jaguar.
-defm : JWriteResFpuPair<WriteVarVecShift, [JFPU01, JVALU], 1>; // NOTE: Doesn't exist on Jaguar.
-defm : JWriteResFpuPair<WriteVarVecShiftY,[JFPU01, JVALU], 1>; // NOTE: Doesn't exist on Jaguar.
 
 ////////////////////////////////////////////////////////////////////////////////
 // Vector insert/extract operations.
 ////////////////////////////////////////////////////////////////////////////////
 
 defm : X86WriteRes<WriteVecInsert,      [JFPU01, JVALU], 7, [1,1], 2>;
 defm : X86WriteRes<WriteVecInsertLd,    [JFPU01, JVALU, JLAGU], 4, [1,1,1], 1>;
 defm : X86WriteRes<WriteVecExtract,     [JFPU0, JFPA, JALU0], 3, [1,1,1], 1>;

 
 def JWriteVZeroIdiom : SchedWriteVariant<[
     SchedVar<MCSchedPredicate<ZeroIdiomPredicate>, [JWriteZeroLatency]>,
     SchedVar<MCSchedPredicate<TruePred>,           [WriteVecLogicX]>
 ]>;
 
 def : InstRW<[JWriteVZeroIdiom], (instrs PXORrr, VPXORrr)>;
 
+// Get unsupported resources which we are not implementing in Jaguar
+//
+include "Unsupported.td"
+
 } // SchedModel