[PATCH] D143645: [llvm] Fix MCSubtargetInfo::checkFeatures to handle unknown features correctly

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu added a comment.

In D143645#4151098 <https://reviews.llvm.org/D143645#4151098>, @craig.topper wrote:

> In D143645#4151078 <https://reviews.llvm.org/D143645#4151078>, @dblaikie wrote:
>
>> In D143645#4150034 <https://reviews.llvm.org/D143645#4150034>, @RamNalamothu wrote:
>>
>>> In D143645#4148087 <https://reviews.llvm.org/D143645#4148087>, @dblaikie wrote:
>>>
>>>> Is this new behavior visible through some existing non-test callers of `MCSubtargetInfo::checkFeatures`? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?
>>>
>>> Without this change, few lit tests will fail with the updated changeset for D140478 <https://reviews.llvm.org/D140478>. Would that be sufficient to showcase that this change is needed?
>>
>> Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?
>
> That's what it looks like to me. I also think that some virtual methods a target can override would be a better way to solve this.

That's what I started with in, the previous versions of D140478 <https://reviews.llvm.org/D140478>, https://reviews.llvm.org/D140478?vs=on&id=484785#toc which @dblaikie wasn't

> In D143645#4151078 <https://reviews.llvm.org/D143645#4151078>, @dblaikie wrote:
>
>> In D143645#4150034 <https://reviews.llvm.org/D143645#4150034>, @RamNalamothu wrote:
>>
>>> In D143645#4148087 <https://reviews.llvm.org/D143645#4148087>, @dblaikie wrote:
>>>
>>>> Is this new behavior visible through some existing non-test callers of `MCSubtargetInfo::checkFeatures`? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?
>>>
>>> Without this change, few lit tests will fail with the updated changeset for D140478 <https://reviews.llvm.org/D140478>. Would that be sufficient to showcase that this change is needed?
>>
>> Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?



In D143645#4151098 <https://reviews.llvm.org/D143645#4151098>, @craig.topper wrote:

> In D143645#4151078 <https://reviews.llvm.org/D143645#4151078>, @dblaikie wrote:
>
>> In D143645#4150034 <https://reviews.llvm.org/D143645#4150034>, @RamNalamothu wrote:
>>
>>> In D143645#4148087 <https://reviews.llvm.org/D143645#4148087>, @dblaikie wrote:
>>>
>>>> Is this new behavior visible through some existing non-test callers of `MCSubtargetInfo::checkFeatures`? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?
>>>
>>> Without this change, few lit tests will fail with the updated changeset for D140478 <https://reviews.llvm.org/D140478>. Would that be sufficient to showcase that this change is needed?
>>
>> Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?
>
> That's what it looks like to me. I also think that some virtual methods a target can override would be a better way to solve this.
>
> In D143645#4151078 <https://reviews.llvm.org/D143645#4151078>, @dblaikie wrote:
>
>> In D143645#4150034 <https://reviews.llvm.org/D143645#4150034>, @RamNalamothu wrote:
>>
>>> In D143645#4148087 <https://reviews.llvm.org/D143645#4148087>, @dblaikie wrote:
>>>
>>>> Is this new behavior visible through some existing non-test callers of `MCSubtargetInfo::checkFeatures`? Could that be tested using a lit-style test to demonstrate that this behavior is desirable/correct at that scope, beyond the narrower unit test?
>>>
>>> Without this change, few lit tests will fail with the updated changeset for D140478 <https://reviews.llvm.org/D140478>. Would that be sufficient to showcase that this change is needed?
>>
>> Could you walk me through that in more detail - oh, is that other patch checking for features in generic code on platforms/targets that don't support those features at all? Maybe that's a problem/the thing that should be fixed, and that other code should be moved to some target-specific hook?

Thanks for the comments.

Even if we find some other way to address the needs of D140478 <https://reviews.llvm.org/D140478>, I think the incorrect behavior of `MCSubtargetInfo::checkFeatures` needs to be fixed.

Is having no existing code referring to the use case scenario being fixed a good reason we leave the incorrect behavior in the code base as is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143645