[PATCH] D121768: [X86][tablgen] Auto-generate trivial fields and trivial interfaces for target features

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D121768#3392153 <https://reviews.llvm.org/D121768#3392153>, @skan wrote:

> In D121768#3391810 <https://reviews.llvm.org/D121768#3391810>, @tmatheson wrote:
>
>>> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121768