[PATCH] D121768: [tablgen][X86] Auto-generate trival fields and interace for target features

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

In D121768#3389065 <https://reviews.llvm.org/D121768#3389065>, @tmatheson wrote:

>> 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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121768/new/

https://reviews.llvm.org/D121768