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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143645