[PATCH] D97916: [Driver][RISCV] Support parsing multi-lib config from GCC.

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng marked 5 inline comments as done.
kito-cheng added a comment.

Here is another solution for flexible multi-lib configuration I consider before:

- Add an option called `-fmultilib-config=`
- Using same or similar syntax with GCC's `--with-multilib-generator`

But the problem is it's really toooooo long for describe a multi-lib config.

e.g: current default bare-metal multi-lib configuration:
`--with-multilib-generator="rv32i-ilp32--c;rv32im-ilp32--c;rv32iac-ilp32--;rv32imac-ilp32--;rv32imafc-ilp32f-rv32imafdc-;rv64imc-lp64--;rv64imfc-lp64f--;rv64imac-lp64--;rv64imafdc-lp64d--"`



================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1625
+                           const std::string &MultilibOutput,
+                           DetectedMultilibs &Result) {
+  llvm::StringSet<> AllABI;
----------------
khchen wrote:
> bool -> static bool?
> const std::string &MultilibOutput -> StringRef ?
> 
> The function name seem like it's not a target-specific function, but it's only support RISC-V now. Maybe we could rename it or make an assertion in function to check the target should be RISC-V?
> 
> Maybe we could rename it or make an assertion in function to check the target should be RISC-V?

Good point, renamed.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1642
+  std::string CurrentArchOpt = Twine("march=", MArch).str();
+  std::string CurrentMCmodelOpt = llvm::StringSwitch<const char *>(CodeModel)
+                                      .Case("medium", "mcmodel=medany")
----------------
khchen wrote:
> Could they be declared as StringRef and use llvm::StringSwitch<StringRef>?
I would prefer keep `const char *` to prevent create a temporal object for `StringRef` here.

And I also found another place are used same way:
```
$ grep "StringSwitch<const" * -R
clang/lib/Basic/SourceManager.cpp:      llvm::StringSwitch<const char *>(BufStr)
clang/lib/Driver/ToolChains/Gnu.cpp:  std::string CurrentMCmodelOpt = llvm::StringSwitch<const char *>(CodeModel)
clang/lib/Driver/ToolChains/Clang.cpp:        MipsTargetFeature = llvm::StringSwitch<const char *>(Value)
clang/lib/Driver/ToolChains/Cuda.cpp:      OOpt = llvm::StringSwitch<const char *>(A->getValue())
clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:      OOpt = llvm::StringSwitch<const char *>(A->getValue())
clang/lib/Driver/ToolChains/Arch/Mips.cpp:    ABIName = llvm::StringSwitch<const char *>(CPUName)
clang/lib/Driver/ToolChains/Arch/Mips.cpp:    CPUName = llvm::StringSwitch<const char *>(ABIName)
clang/lib/Driver/ToolChains/Arch/PPC.cpp:    return llvm::StringSwitch<const char *>(CPUName)
clang/lib/Driver/ToolChains/Arch/PPC.cpp:  return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Arch/Sparc.cpp:    return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Arch/Sparc.cpp:    return llvm::StringSwitch<const char *>(Name)
clang/lib/Driver/ToolChains/Darwin.cpp:  return llvm::StringSwitch<const char *>(Arch)
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:        StringSwitch<const char *>(T.first->getValueAsString("AccessQualifier"))
clang/utils/TableGen/ClangOpenCLBuiltinEmitter.cpp:      OS << StringSwitch<const char *>(
llvm/include/llvm/Support/FormatProviders.h:    Stream << StringSwitch<const char *>(Style)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:          StringSwitch<const char *>(BroadcastString)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:    const char *Repl = StringSwitch<const char *>(Name)
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:  const char *Repl = StringSwitch<const char *>(Op.getToken())
llvm/lib/Support/Host.cpp:  return StringSwitch<const char *>(StringRef(CPUStart, CPULen))
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/lib/Support/Host.cpp:    return StringSwitch<const char *>(Part)
llvm/unittests/Support/Host.cpp:  StringRef MCPU = StringSwitch<const char *>(CPU)
```


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1650
+
+  if (File) {
+    SmallVector<StringRef, 128> Lines;
----------------
khchen wrote:
> ```
> if (!File)
>   return;
> ```
Good suggestion, thanks! 


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1703
+        }
+      }
+      Ms.emplace_back(Multilib);
----------------
Jim wrote:
> Do you have plan to support other kind of options to build multilib?
Currently, no, but I think could be consider in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97916