This is an archive of the discontinued LLVM Phabricator instance.

Move SubtargetFeature.h from MC to TargetParser
ClosedPublic

Authored by jobnoorman on May 15 2023, 2:19 AM.

Details

Summary

SubtargetFeature.h is currently part of MC while it doesn't depend on
anything in MC. Since some LLVM components might have the need to work
with target features without necessarily needing MC, it might be
worthwhile to move SubtargetFeature.h to a different location. This will
reduce the dependencies of said components.

Note that I choose TargetParser as the destination because that's where
Triple lives and SubtargetFeatures feels related to that. Suggestions
for alternative locations are welcome.

This issues came up during a JITLink review (D149522). JITLink would
like to avoid a dependency on MC while still needing to store target
features.

I'm not entirely sure who should review this patch so please add more
reviewers if necessary.

Diff Detail

Event Timeline

jobnoorman created this revision.May 15 2023, 2:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 2:19 AM
Herald added subscribers: bviyer, asb, luke and 60 others. · View Herald Transcript
jobnoorman requested review of this revision.May 15 2023, 2:19 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMay 15 2023, 2:19 AM

FWIW I'm in favor of this: as @jobnoorman mentioned it'd eliminate JITLink's library dependence on MC.

arsenm accepted this revision.May 16 2023, 12:31 AM
This revision is now accepted and ready to land.May 16 2023, 12:31 AM

This seems fine, but please consider making MC/SubtargetFeature.h a forwarding header temporarily.

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

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?

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

MaskRay accepted this revision.May 17 2023, 5:32 PM

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

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

Thanks for the link! If I understand the discussion there correctly, the motivation for adding the forwarding header was to not have to update any #include sites directly in order to reduce the size of the patch. So what you are asking me to do is

  1. Update this patch to have a forwarding header and not change #include sites;
  2. Create a second patch that updates #includes and removes the forward. This patch would be committed at some point in the future once we're sure the move is fine.

Is this correct?

I do wonder if this is necessary in this case as the patch is much smaller than the one in the link you shared.

Friendly ping. Any thoughts on my previous comment @MaskRay?

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

Thanks for the link! If I understand the discussion there correctly, the motivation for adding the forwarding header was to not have to update any #include sites directly in order to reduce the size of the patch. So what you are asking me to do is

  1. Update this patch to have a forwarding header and not change #include sites;
  2. Create a second patch that updates #includes and removes the forward. This patch would be committed at some point in the future once we're sure the move is fine.

Is this correct?

I do wonder if this is necessary in this case as the patch is much smaller than the one in the link you shared.

If we are certain this will not be reverted and cause churn, making the header switch in one single patch looks fine to me...

I have checked that this patch has migrated things that might be neglected: unittest directories, openmp,mlir,clang, etc.

If we are certain this will not be reverted and cause churn, making the header switch in one single patch looks fine to me...

Great. I'm not sure if we can ever be a 100% certain it will not get reverted. To increase our chances, I'll wait a bit before landing this. Does anyone else have thoughts about this patch?

I have checked that this patch has migrated things that might be neglected: unittest directories, openmp,mlir,clang, etc.

Thanks for checking this!

This revision was landed with ongoing or failed builds.Jun 26 2023, 2:20 AM
This revision was automatically updated to reflect the committed changes.