[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

Mon Oct 18 12:38:58 PDT 2021

teemperor added a comment.

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

> In D105168#3069499 <https://reviews.llvm.org/D105168#3069499>, @teemperor wrote:
>
>> This broke the modules build:
>>
>>   While building module 'LLVM_Utils' imported from lvm/lib/Support/TargetParser.cpp:14:
>>   In file included from <module-includes>:209:
>>   llvm/include/llvm/Support/RISCVISAInfo.h:15:10: fatal error: could not build module 'LLVM_Option'
>>   #include "llvm/Option/ArgList.h"
>>    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>>   llvm/lib/Support/TargetParser.cpp:14:10: fatal error: could not build module 'LLVM_Utils'
>>   #include "llvm/Support/TargetParser.h"
>>    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> `Support` headers can't include `Option` headers (as `Option` depends on `Support` already, so that would be a cyclic dependency). I believe folks here are in the US time zone, so I went ahead and replaced the header include with a forward decl to unbreak the bots (see de4d2f80b75e2a1e4b0ac5c25e20f20839633688 <https://reviews.llvm.org/rGde4d2f80b75e2a1e4b0ac5c25e20f20839633688> )
>>
>> FWIW, I think there is a better place than `Support` for this new class. `Support` is a dependency of nearly every single LLVM component, but this class seems to be used by a handful of files. Could we maybe move this to some other/new part of LLVM (and make the few components that need it depend on that)?
>
> Thanks for the header fix. I think we also need to fix the library circular dependency that still exists. The Support library should not use anything from the Option library. Maybe we can partition the function some way that moves the MakeArgStr back into the clang driver code.
>
> I don't know if we have a better library for this new class. I think Support is the only common library between the clang driver and LLVM's MC layer. I supposed we could create a new library, but that might be a hard sell for one file.

IIRC there are some other target-specific classes in Support for the same reason. Maybe we can make something such as `TargetSupport` or `SharedTarget`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168