JonChesterfield 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)));
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98746/new/
https://reviews.llvm.org/D98746
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
