[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay accepted this revision. MaskRay added a comment. Please check these builds all work: - default - `-DLLVM_LINK_LLVM_DYLIB=on` - `-DBUILD_SHARED_LIBS=on` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

fpetrogalli accepted this revision. fpetrogalli added a comment. This revision is now accepted and ready to land. Thank you for working on this @lenary - LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay added a comment. The name `llvm/lib/TargetParser` LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/ https://reviews.llvm.org/D137838 ___ cfe-commits mailing list

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

lenary added a comment. > As for the name TargetParser. @arsenm came up with the name > SubtargetRegistry. I am fine with either names. @fpetrogalli I'm going to stick with TargetParser, because I think this is less confusing, given that TargetRegistry is already a component. Repository:

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

lenary added inline comments. Comment at: clang/lib/Lex/CMakeLists.txt:3 -set(LLVM_LINK_COMPONENTS support) +set(LLVM_LINK_COMPONENTS + Support fpetrogalli wrote: > I wonder how `support` was not being detected as a linking error... Not sure, but I still

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

fpetrogalli added a comment. @lenary - thank you for the update! I have added a bunch of miso comments, a bit repetitive... by the time I realised they were repetitive it was faster to get to the bottom of it than removing them! In D137838#3931295 ,

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

lenary added a comment. I've updated the patch to use forwarding headers (and to rebase past some changes that have happened in the interim). The patch is still huge because of the number of places using the TargetParsers, which need the component added to their CMakeLists.txt. I think after

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

MaskRay added a comment. Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit when more stuff of this kind was introduced into LLVMSupport. +1 for adding temporary forwarding headers for now to avoid update all `#include` users. Repository: rG LLVM Github Monorepo

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

fpetrogalli added a comment. Hi @lenary - thank you for working on this! The patch is reasonable to me. I agree on the suggestion of using forwarding headers or the first iteration of the change, it will make it easier to review. I'll adapt the auto-get patch at D137517

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

lenary added a comment. In D137838#3921828 , @thakis wrote: > This is a gigantic diff. I'd recommend keeping the .h files in the old place > for v0 and make them just forwarding headers that include the header at the > new location. That way, you don't

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

thakis added a comment. This is a gigantic diff. I'd recommend keeping the .h files in the old place for v0 and make them just forwarding headers that include the header at the new location. That way, you don't have to update oodles of include lines in this patch, and it makes it a bit easier

[PATCH] D137838: [RFC][Support] Move TargetParsers to new component

lenary added a comment. Herald added a subscriber: Michael137. Apologies to everyone who has automatically been marked as a reviewer for this change. There is more context for it here: https://discourse.llvm.org/t/targetparser-auto-generation-of-riscv-cpu-definitions/66419/4?u=lenary