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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
