jdoerfert added inline comments.
================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124 + // Currently defaults to 3 in AMDGPUBaseInfo.cpp + // Using that default lets clang emit IR for amdgcn when llvm has been built + // without that target, provided the user wants this code object version + if (CodeObjVer != 3) { + CmdArgs.insert(CmdArgs.begin() + 1, + Args.MakeArgString(Twine("--amdhsa-code-object-version=") + + Twine(CodeObjVer))); ---------------- JonChesterfield wrote: > t-tye wrote: > > This seem rather fragile. If the backend default changes and this code is > > not updated it will likely result in creating the wrong code object version. > That is true, though numbers 2 through 4 are used in > getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too. > > Equally, today if we change the default in clang and forget to update the > backend, we won't notice because we always pass this argument. > > What we have at present is the back end has the state and the front end > chooses the default. Needing to write '3' in both places is a symptom of > that. Perhaps the back end should not have a default for it, requiring this > to always be passed, at which point we are going to have a bad time building > amdgpu openmp by default. > > This change decouples the two for the (likely common) case where the user has > not chosen to override the value. It's not ideal, but equally we don't change > the version that often and there are lit tests that notice if we get it > wrong. Plus stuff seems to break if the code object version is not as > expected. > > There may be a better solution. Target specific variable set by clang feels > like something that will occur elsewhere. Arguably, the backend should have a default, the option should be used by users that don't like the default. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98746/new/ https://reviews.llvm.org/D98746 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits