[PATCH] D124721: [OpenMP] Allow compiling multiple target architectures with OpenMP

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:313
+      if (BoundArch.empty())
+        checkSystemForAMDGPU(Args, *this, Arch);
       DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_march_EQ), Arch);
----------------
tra wrote:
> I'd change `checkSystemForAMDGPU` to return the Arch or empty string.
> 
> I'd also simplify the code to something like this:
> ```
> std::string Arch = DAL->getLastArgValue(options::OPT_march_EQ).str();
> if (Arch.empty()) {
>   Arch = !BoundArch.empty() ? BoundArch :  checkSystemForAMDGPU(Args, *this);
>   DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_march_EQ), Arch);
> }
> ```
Having `checkSystemForAMDGPU` cause errors when trying to test this was a pain. It'll take a bit more plumbing to address that so I think I'll leave this as-is for this patch and address it in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124721