[PATCH] D150549: Move SubtargetFeature.h from MC to TargetParser

Wed May 17 17:32:23 PDT 2023

MaskRay accepted this revision.
MaskRay added a comment.

In D150549#4349531 <https://reviews.llvm.org/D150549#4349531>, @jobnoorman wrote:

> In D150549#4347056 <https://reviews.llvm.org/D150549#4347056>, @MaskRay wrote:
>
>> This seems fine, but please consider making `MC/SubtargetFeature.h` a forwarding header temporarily.
>
> To be clear: do you want me to check-in this forwarding header or should I just do it locally for the tests below? If the former, what exactly is the purpose of adding this header?

The motivation is similar to https://reviews.llvm.org/D137838#3921828

> That way, you don't have to update oodles of include lines in this patch, and it makes it a bit easier to see what's going on. (You can then update all the include lines in a trivial follow-up if this change goes through, and then remove the forwarding headers in Support, to cut the dependency you want to remove.)

(There is a non-zero probability that the patch may miss something and get reverted. By creating a forwarding header we don't risk causing too much churn to the many files.)
For a large-scaling refactoring

>> Our official build system CMake doesn't do layering checking for headers (https://llvm.org/docs/CodingStandards.html#library-layering).
>>
>> Our unofficial build system Bazel supports layering checking <http://maskray.me/blog/2022-09-25-layering-check-with-clang>. You may edit `utils/bazel/llvm-project-overlay/llvm/BUILD.bazel` and see whether bazel reports a circular dependency. I think this patch is fine, but just in case.
>
> I did the following:
>
> - Add the forwarding header
> - Update `BUILD.bazel` (I only had to add the `TargetParser` dependency to `llvm-tblgen` to make this work; should I add this change to this patch?)
> - Run `bazel build '@llvm-project//llvm:all'` (using `clang` from `main`).
>
> This worked fine. Is this what you meant with the circular dependency check? (I'm not familiar with bazel so I might be missing something.)

Thank you for verification. Then I think this is good. `layering_check` is the default, so this verifies that we are good.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150549