[PATCH] D104589: [llvm-rc] Don't rewrite the arch in the default triple unless necessary

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:148
 
-Triple::ArchType getDefaultArch(Triple::ArchType Arch) {
+bool isUsableDefaultArch(Triple::ArchType Arch) {
   switch (Arch) {
----------------
amccarth wrote:
> Should this be just `isUsableArch`?  It doesn't seem like `Default` is relevant in this new formulation.
Sure, I guess I could leave that word out


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:184
+  if (!isUsableDefaultArch(T.getArch()))
+    T.setArch(getDefaultFallbackArch());
   if (T.isWindowsGNUEnvironment())
----------------
amccarth wrote:
> Is there a guarantee that the fallback will be usable?
The main criterion for usable here practically is whether “clang -target <arch>-windows -E” works as expected, i.e. preprocesses with _WIN32 defined. Clang does handle that for those four arches even if the corresponding target lowering backend is disabled.

Or put another way, we’ve got tests within clang that try to do preprocessing by just calling llvm-rc or llvm-windres without specifying any target arch, and checking that the preprocessing works a as expected (defining _WIN32 and RC_DEFINED - the latter comes from a parameter passed from llvm-rc). If llvm defaults to e.g. ppc64, then we must override the arch and do the preprocessing test with e.g. an x86_64 triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104589