This is an archive of the discontinued LLVM Phabricator instance.

[X86][tablgen] Auto-generate trivial fields and trivial interfaces for target features
AbandonedPublic

Authored by skan on Mar 15 2022, 8:36 PM.

Details

Summary

A trivial field is always zero-initialized. An interface is trivial if directly returns
a field of the object or compares a field w/ a constant value, e.g

bool hasX87() const { return HasX87; }
bool hasSSE1() const { return X86SSELevel >= SSE1; }

The effort of writing such code can be saved.

We start with X86 target features in this patch, and do the similar things for other
archs in the following patches.

Diff Detail

Event Timeline

skan created this revision.Mar 15 2022, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 8:36 PM
skan requested review of this revision.Mar 15 2022, 8:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 8:36 PM

If the patch contains changes to X86 files, please put X86 in the title. I have email filters that check the subject.

Can you split the NFC renames into their own patch?

craig.topper added inline comments.Mar 15 2022, 9:07 PM
llvm/utils/TableGen/SubtargetEmitter.cpp
1743

You can do

if (!Attrs.insert(Attribute).second)
  continue;

instead of calling find and insert.

pengfei added inline comments.Mar 15 2022, 10:00 PM
llvm/lib/Target/X86/X86.td
22

I think In64BitMode is more readable, we usually generate 32 bit instructions under 64 bit mode.

33

How about define a new class like TrivalSubtargetFeature so that we don't need to add , [], 0 to the old one?

llvm/lib/Target/X86/X86InstrInfo.td
882

Should change here too? The same below.

llvm/lib/Target/X86/X86Subtarget.cpp
56

Unrelated change.

llvm/lib/Target/X86/X86Subtarget.h
89

Some comments like this doesn't appear on X86.td, should we move all these comments there?

skan added a comment.Mar 15 2022, 10:34 PM

If the patch contains changes to X86 files, please put X86 in the title. I have email filters that check the subject.

Can you split the NFC renames into their own patch?

Done. Splited the NFC change to https://reviews.llvm.org/rG052d37dc7ced.

skan retitled this revision from [tablgen] Auto-generate fields & interfaces for target features to [tablgen][X86] Auto-generate fields & interfaces for target features.Mar 15 2022, 10:40 PM
skan updated this revision to Diff 415708.Mar 15 2022, 11:34 PM
skan marked 3 inline comments as done.

Address comments

skan added inline comments.Mar 15 2022, 11:47 PM
llvm/lib/Target/X86/X86.td
22

It's meaningless to have different name for inteface and the member. We already use the interface is64Bit.

33

If so, then we have to replace most of SubtargetFeature with TrivalSubtargetFeature. Now we only need to add , [], 0 for CMOV, so I think TrivalSubtargetFeature is
not worthy.

skan updated this revision to Diff 415711.Mar 16 2022, 12:18 AM
skan marked an inline comment as done.

Add class NonTrivalSubtargetFeature

Matt added a subscriber: Matt.Mar 16 2022, 7:36 AM
skan retitled this revision from [tablgen][X86] Auto-generate fields & interfaces for target features to [tablgen][X86] Auto-generate fields and trival interace for target features.Mar 16 2022, 10:57 PM
skan edited the summary of this revision. (Show Details)
skan edited the summary of this revision. (Show Details)

This looks remarkably similar to an ARM/AArch64 patch I've had up for a couple of weeks: D120906

The main differences seem to be:

  1. I use a more general GET_SUBTARGETINFO_MACRO macro that can be expanded for other uses going forwards, rather than emitting the various bits of C++ directly.
  2. This patch makes a distinction between trivial/nontrivial members, why is this necessary?
skan updated this revision to Diff 416133.Mar 17 2022, 4:11 AM
  1. Add three classes: TrivalFieldSubtargetFeature, TrivalInterfaceSubtargetFeature, TrivalSubtargetFeature for flexibilty
  2. Add some comments
  3. Move the description of feature to the interface if there are multiples features related to the field
skan added a comment.Mar 17 2022, 4:17 AM

This looks remarkably similar to an ARM/AArch64 patch I've had up for a couple of weeks: D120906

The main differences seem to be:

  1. I use a more general GET_SUBTARGETINFO_MACRO macro that can be expanded for other uses going forwards, rather than emitting the various bits of C++ directly.
  2. This patch makes a distinction between trivial/nontrivial members, why is this necessary?

I'm sorry that I didn't notice your patch, otherwise I would comment on it rather than propose a patch. Let me check the difference.

skan added a comment.EditedMar 17 2022, 4:55 AM

This looks remarkably similar to an ARM/AArch64 patch I've had up for a couple of weeks: D120906

The main differences seem to be:

  1. I use a more general GET_SUBTARGETINFO_MACRO macro that can be expanded for other uses going forwards, rather than emitting the various bits of C++ directly.
  2. This patch makes a distinction between trivial/nontrivial members, why is this necessary?

I'm sorry that I didn't notice your patch, otherwise I would comment on it rather than propose a patch. Let me check the difference.

There are more differences

  1. I copy the descriptions of the features to the generated header file but you just omit them
  2. I also support the enum types, e.g
bool hasAVX512() const { return X86SSELevel >= AVX512; }
  1. This patch makes a distinction between trivial/nontrivial members & interface b/c some targets has some tricky interfaces, e.g
`
bool hasCMov() const { return HasCMov || X86SSELevel >= SSE1 || is64Bit(); }
bool useAA() const override { return UseAA; }
bool hasLAHFSAHF() const { return HasLAHFSAHF64 || !is64Bit(); }

It seems that you patch handle them incorrectly by now.

  1. A field may be set to false by a feature while the default value of the field can either be true or false at the same time. So I provide a way to avoid generating field that is not zero-initialized. But your assumption is that the default value of the field must be true if a feature set it to false.
skan retitled this revision from [tablgen][X86] Auto-generate fields and trival interace for target features to [tablgen][X86] Auto-generate trival fields and interace for target features.Mar 17 2022, 5:07 AM

This patch makes a distinction between trivial/nontrivial members & interface b/c some targets has some tricky interfaces, e.g

bool hasCMov() const { return HasCMov || X86SSELevel >= SSE1 || is64Bit(); }
bool useAA() const override { return UseAA; }
bool hasLAHFSAHF() const { return HasLAHFSAHF64 || !is64Bit(); }

It seems that you patch handle them incorrectly by now.

It would be better to take this opportunity to fix these inconsistencies between the getter methods and the fields, rather than adding complexity to the tablegen to keep them. If HasCMov is false it is counterintuitive for hasCMov to return true. I handled this by the getter always matching the field name, e.g.

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }
bool useNEONForSinglePrecisionFP() const { return hasNEON() && hasNEONForFP(); }

A field may be set to false by a feature while the default value of the field can either be true or false at the same time. So I provide a way to avoid generating field that is not zero-initialized. But your assumption is that the default value of the field must be true if a feature set it to false.

How many examples of this are there, and are they intentional? This also seems like an inconsistency that could lead to confusion. If a field has default value false and enabling the feature sets it to false, at what point is it set to true and which takes precedence?

skan added a comment.Mar 17 2022, 6:02 AM

This patch makes a distinction between trivial/nontrivial members & interface b/c some targets has some tricky interfaces, e.g

bool hasCMov() const { return HasCMov || X86SSELevel >= SSE1 || is64Bit(); }
bool useAA() const override { return UseAA; }
bool hasLAHFSAHF() const { return HasLAHFSAHF64 || !is64Bit(); }

It seems that you patch handle them incorrectly by now.

It would be better to take this opportunity to fix these inconsistencies between the getter methods and the fields, rather than adding complexity to the tablegen to keep them. If HasCMov is false it is counterintuitive for hasCMov to return true. I handled this by the getter always matching the field name, e.g.

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }
bool useNEONForSinglePrecisionFP() const { return hasNEON() && hasNEONForFP(); }

A field may be set to false by a feature while the default value of the field can either be true or false at the same time. So I provide a way to avoid generating field that is not zero-initialized. But your assumption is that the default value of the field must be true if a feature set it to false.

How many examples of this are there, and are they intentional? This also seems like an inconsistency that could lead to confusion. If a field has default value false and enabling the feature sets it to false, at what point is it set to true and which takes precedence?

I don't why the inconsistencies are there, probably they are a bugfix or a workaround themselves... In another view, I think we should give the developer such flexibility to customize the interface.
Of course the inner-class initializer comes first, then the value set by feature overrides the default value in function ParseSubtargetFeatures.

The cmov mess is because SSE and 64-bit odegen assumes cmov exists. We couldn’t make 64-bit or SSE imply cmov feature or -mattr=-cmov would disable SSE or 64bit which is just completely broken. Thus the OR that ignores attempts to disable cmov.

llvm/include/llvm/Target/Target.td
1647

Trival->Trivial

Every feature bit is exposed directly to FeatureBits vector at the MC layer using the name of the tablegen def. It is to our advantage to have consistent naming between the Def, the Subtarget field name, and the getter. Thus the concept of the getter being different than the Subtarget field doesn’t make sense because it means the MC layer is still wrong.

skan updated this revision to Diff 416382.Mar 17 2022, 8:31 PM
skan marked an inline comment as done.

Fix typo Trival->Trivial

skan retitled this revision from [tablgen][X86] Auto-generate trival fields and interace for target features to Auto-generate trivial fields and trivial interfaces for target features.Mar 17 2022, 8:34 PM
skan edited the summary of this revision. (Show Details)
skan added a comment.Mar 17 2022, 9:02 PM

The cmov mess is because SSE and 64-bit odegen assumes cmov exists. We couldn’t make 64-bit or SSE imply cmov feature or -mattr=-cmov would disable SSE or 64bit which is just completely broken. Thus the OR that ignores attempts to disable cmov.

Thank you for the explanation, craig! Then I think it's important to have the flexibility to customize the interface, at least for now. Similar interfaces also exist in AMDGPU target.

craig.topper added inline comments.Mar 17 2022, 9:22 PM
llvm/lib/Target/X86/X86.td
106

Doesn't hasCmpxchg16b() have non-trivial implementation?

craig.topper added inline comments.Mar 17 2022, 9:42 PM
llvm/lib/Target/X86/X86.td
106

Nevermind I see this uses TrivialFieldSubtargetFeature

skan updated this revision to Diff 416394.Mar 17 2022, 11:11 PM
skan edited the summary of this revision. (Show Details)

Rebase

skan retitled this revision from Auto-generate trivial fields and trivial interfaces for target features to [X86][tablgen] Auto-generate trivial fields and trivial interfaces for target features.Mar 17 2022, 11:14 PM

Then I think it's important to have the flexibility to customize the interface, at least for now.

To be clear, I think that flexibility is a *bad* thing in this situation and not justified. Autogenerated code is already kind of magic, so it should be repetitive, boilerplate and consistent. Allowing people to tweak the code generation to behave inconsistently some of the time is a recipe for confusion. It will also encourage more deviation in future. If this is to be used in other backends as well, I would prefer a solution that resulted in a 1-1 relationship between getter and field. IIUC, this is in agreement with what @craig.topper is saying here:

Every feature bit is exposed directly to FeatureBits vector at the MC layer using the name of the tablegen def. It is to our advantage to have consistent naming between the Def, the Subtarget field name, and the getter. Thus the concept of the getter being different than the Subtarget field doesn’t make sense because it means the MC layer is still wrong

To give a concrete example using CMov again, either the feature or the existing getter can be simply renamed, making it clear that hasCMov() is not simply checking for the feature:

/// Autogenerated getter for the autogenerated field CMovFeature (not necessarily a good name).
/// Renamed to avoid changing the name of hasCMov()
bool hasCMovFeature() const { return HasCMovFeature; }

/// People still have the flexibility to create a custom interface in a way that is explicit and obvious to the reader
bool hasCMov() const { return hasCMov() || X86SSELevel >= SSE1 || is64Bit(); }

Given the above, I haven't seen any examples to justify the added complexity to the tablegen classes.

skan added a comment.Mar 18 2022, 2:49 AM

Then I think it's important to have the flexibility to customize the interface, at least for now.

To be clear, I think that flexibility is a *bad* thing in this situation and not justified. Autogenerated code is already kind of magic, so it should be repetitive, boilerplate and consistent. Allowing people to tweak the code generation to behave inconsistently some of the time is a recipe for confusion. It will also encourage more deviation in future. If this is to be used in other backends as well, I would prefer a solution that resulted in a 1-1 relationship between getter and field. IIUC, this is in agreement with what @craig.topper is saying here:

Every feature bit is exposed directly to FeatureBits vector at the MC layer using the name of the tablegen def. It is to our advantage to have consistent naming between the Def, the Subtarget field name, and the getter. Thus the concept of the getter being different than the Subtarget field doesn’t make sense because it means the MC layer is still wrong

To give a concrete example using CMov again, either the feature or the existing getter can be simply renamed, making it clear that hasCMov() is not simply checking for the feature:

/// Autogenerated getter for the autogenerated field CMovFeature (not necessarily a good name).
/// Renamed to avoid changing the name of hasCMov()
bool hasCMovFeature() const { return HasCMovFeature; }

/// People still have the flexibility to create a custom interface in a way that is explicit and obvious to the reader
bool hasCMov() const { return hasCMov() || X86SSELevel >= SSE1 || is64Bit(); }

Given the above, I haven't seen any examples to justify the added complexity to the tablegen classes.

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface. In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

skan added a comment.EditedMar 18 2022, 5:53 AM

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Besides, I think adding two bits to a class is very common in a DSL (tablgen). It's worthy b/c flexibility is kept and code is reduced at same time.

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

Is useAA() really supported. It's off by default, nothing implies FeatureUseAA, and no lit tests pass -mattr=+use-aa. Seems like we should look at it's history, it might be a candidate for deletion.

skan added a comment.EditedMar 18 2022, 8:07 PM

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

I don't understand these use cases. How often are you doing these kinds of things? The dependencies in the compiler are supposed to be as loose as possible. BMI2 doesn't imply BMI1 for example.

skan added a comment.Mar 18 2022, 8:19 PM

Also, D120906 drops these comments in the header file by moving them to TD file. I'm not sure whether there is a document issue. We keep the comments in the generated header file, which seems better.

skan added a comment.Mar 18 2022, 8:27 PM

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

I don't understand these use cases. How often are you doing these kinds of things? The dependencies in the compiler are supposed to be as loose as possible. BMI2 doesn't imply BMI1 for example.

But AVX implies SSE. In fact, it's the common case used in my development. Why shouldn't we have such usage?

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

I don't understand these use cases. How often are you doing these kinds of things? The dependencies in the compiler are supposed to be as loose as possible. BMI2 doesn't imply BMI1 for example.

But AVX implies SSE. In fact, it's the common case used in my development. Why shouldn't we have such usage?

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

skan added a comment.Mar 18 2022, 8:45 PM

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

I don't understand these use cases. How often are you doing these kinds of things? The dependencies in the compiler are supposed to be as loose as possible. BMI2 doesn't imply BMI1 for example.

But AVX implies SSE. In fact, it's the common case used in my development. Why shouldn't we have such usage?

Let me tell another scenario. We could have different front ends for LLVM, e,g, clang, flang, aocc or anything else. Some of them are open-source and some of them are not. We could use these front ends to emit the LLVM IR, then use LLVM middle/back end to optimize and tranlate it. New features can be transparent to the front end if backend can override the target features. I have to say that "-mattr" is hard to use or not usable when LTO is enabled. At this time, adding knob in the backend is pretty useful. Flexibility is quite important.

Your method can not get rid of non-trivial feature like useAA, which has an virtual interface.

I gave an example above of exactly this:

bool enablePostRAScheduler() const override { return usePostRAScheduler(); }

In addition, as a LLVM backend developer, I often need to hack these interfaces to do some performance tuning or testing work, so the flexibility is quite important.

Could you be more specific? I can't imagine how consistent getter/field naming prevents performance or testing work.

Still seeing no justification for the added complexity of 2 new fields to SubtargetFeature, 3 new SubtargetFeature subclasses, in order to avoid a couple of function renames. We already have custom predicates.

Let me tell the scenario:

The features defined in TD file have dependencies between each other. The simpliest case is that both feature A and feature B depends on feature C due to any historic HW reason. When we disable C by passing
knobs like -mattr=-C or -mnoC, feature A and B will be disabled too. However, it's possible either A or B does not depends on C in SW, namely, compiler could emit ISA in A while not emitting ISA in C.
Handwritten (non-trivial) interface gives us an quick way to untie such dependencies. If we'd like to disable C w/o affecting A and B, we can simply write such code

bool hasC() const { return false; }

That's the flexibility and we could have more complicated cases.

Since you would have to edit both the .td to make it non-trivial and then edit the .h file. Couldn't you instead rename the field in the .td so the autogenerated getter has a different name, and then add the custom getter using the old name?

It's not straightforward vs my method. As I said, this is the simpliest case, non-trival interfaces allow us to untie and retie the dependencies in more complicated cases. Another example is that we could add a knob like -force-noC to the backend, then define the interface like

bool hasC() const { return C && !ForceNoC; }

so that we can control the dependency w/o building two compilers.

I don't understand these use cases. How often are you doing these kinds of things? The dependencies in the compiler are supposed to be as loose as possible. BMI2 doesn't imply BMI1 for example.

But AVX implies SSE. In fact, it's the common case used in my development. Why shouldn't we have such usage?

Let me tell another scenario. We could have different front ends for LLVM, e,g, clang, flang, aocc or anything else. Some of them are open-source and some of them are not. We could use these front ends to emit the LLVM IR, then use LLVM middle/back end to optimize and tranlate it. New features can be transparent to the front end if backend can override the target features. I have to say that "-mattr" is hard to use or not usable when LTO is enabled. At this time, adding knob in the backend is pretty useful. Flexibility is quite important.

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

skan added a comment.Mar 18 2022, 9:07 PM

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

skan added a comment.Mar 18 2022, 9:08 PM

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

Some front-ends are not open-source. We could not teach it.

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

Some front-ends are not open-source. We could not teach it.

If we provide good library interfaces the non open source projects can implement it themselves.

There’s no guarantee that a way to set an llvm override exists in another frontend. Last I knew ispc for example didn’t have an equivalent of -mllvm. I think I heard once that Apples version of clang doesn’t support -mllvm.

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

skan added a comment.EditedMar 18 2022, 9:34 PM

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

Some front-ends are not open-source. We could not teach it.

If we provide good library interfaces the non open source projects can implement it themselves.

There’s no guarantee that a way to set an llvm override exists in another frontend. Last I knew ispc for example didn’t have an equivalent of -mllvm. I think I heard once that Apples version of clang doesn’t support -mllvm.

We don't have such "good library interfaces" by now, so we should keep the flexibility until that. At least for now, we could not force a non open source front end to support that plugin.

skan added a comment.Mar 18 2022, 9:49 PM

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

I agree that the correct approach is to relax the dependencies too. But it takes more effort and not quick enough.

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

I agree that the correct approach is to relax the dependencies too. But it takes more effort and not quick enough.

I'm saying that a dependency currently exists, that changing the subtarget "has" function will most likely result in a crash or a miscompile without other changes. Do you disagree with that?

skan added a comment.Mar 18 2022, 10:00 PM

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

I agree that the correct approach is to relax the dependencies too. But it takes more effort and not quick enough.

I'm saying that a dependency currently exists, that changing the subtarget "has" function will most likely result in a crash or a miscompile without other changes. Do you disagree with that?

I agree, but we can improve.

craig.topper added a comment.EditedMar 18 2022, 10:01 PM

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

I agree that the correct approach is to relax the dependencies too. But it takes more effort and not quick enough.

I'm saying that a dependency currently exists, that changing the subtarget "has" function will most likely result in a crash or a miscompile without other changes. Do you disagree with that?

I agree, but we can improve.

Then why does it need to be super easy to change the "has" function if it will most likely require more work to make that change functional?

skan added a comment.Mar 18 2022, 10:43 PM

But I guarantee the compiler will break if you force SSE to false and keep AVX. So that’s not a reasonable case.

It's a check in X86ISelLowering.cpp, we can relax it.

Yes, but if you did that work then you should probably change the avx to not imply sse.

The dependencies we have are largely due to implementation details of the features within the compiler. If we relax those implementation details we should relax the dependencies. For the most part I don’t think you can just relax a dependency without making additional code changes.

AVX512 implementation has quite a few special cases to avoid assuming dependencies except where it was really necessary. It would be simpler if avx512bw implied avx512 dq for example. But no documentation exists of dependencies.

I agree that the correct approach is to relax the dependencies too. But it takes more effort and not quick enough.

I'm saying that a dependency currently exists, that changing the subtarget "has" function will most likely result in a crash or a miscompile without other changes. Do you disagree with that?

I agree, but we can improve.

Then why does it need to be super easy to change the "has" function if it will most likely require more work to make that change functional?

I agree this case is not so reasonable.

skan added a comment.Mar 18 2022, 10:45 PM

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

Some front-ends are not open-source. We could not teach it.

If we provide good library interfaces the non open source projects can implement it themselves.

There’s no guarantee that a way to set an llvm override exists in another frontend. Last I knew ispc for example didn’t have an equivalent of -mllvm. I think I heard once that Apples version of clang doesn’t support -mllvm.

We don't have such "good library interfaces" by now, so we should keep the flexibility until that. At least for now, we could not force a non open source front end to support that plugin.

How about this case?

skan added a comment.Mar 19 2022, 2:47 AM

So you want feature specific knobs that you have to pass through those front end command line interfaces with something like clang’s -mllvm?

In my opinion we should add more interfaces in X86TargetParser.cpp so that all frontends can learn what features llvm supports without a hardcoded list like in Options.td. And those frontends should use the target-feature attribute.

Some front-ends are not open-source. We could not teach it.

If we provide good library interfaces the non open source projects can implement it themselves.

There’s no guarantee that a way to set an llvm override exists in another frontend. Last I knew ispc for example didn’t have an equivalent of -mllvm. I think I heard once that Apples version of clang doesn’t support -mllvm.

We don't have such "good library interfaces" by now, so we should keep the flexibility until that. At least for now, we could not force a non open source front end to support that plugin.

How about this case?

Never mind. I could do some work to enable somthing like "-mattr" when LTO is enabled in theory, so non-trivial getter is not necessary.

I have one last question for @RKSimon :

D120906 drops comments in the header file by moving them to TD file. Is there any document issue due to this?

If there is, I think we should use tablgen to emit C++ bits directly rather than use macro to expand the code.
If it's not a issue, I'm tempted to abandon this pacth and do similar things for X86 like D120906.

skan added a comment.Mar 19 2022, 3:42 AM
This comment was removed by skan.
skan abandoned this revision.Mar 22 2022, 5:05 AM

Thanks all for the discussion!