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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits