Page MenuHomePhabricator

Removing unsupported resources from sched model
AbandonedPublic

Authored by avt77 on Jun 5 2018, 3:56 AM.

Details

Reviewers
RKSimon
courbet
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.

Diff Detail

Event Timeline

avt77 created this revision.Jun 5 2018, 3:56 AM

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 updated this revision to Diff 150139.Jun 6 2018, 8:13 AM

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.

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 added a comment.EditedJun 6 2018, 8:50 AM

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?

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 added a comment.EditedJun 7 2018, 1:00 AM

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?

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.

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 added a comment.EditedJun 7 2018, 6:55 AM

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.

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.

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

avt77 abandoned this revision.Jul 24 2018, 12:41 AM

The main idea of this patch is implemented in D47763.