yaxunl 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)));
----------------
jdoerfert wrote:
> 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.
>
I think a reasonable behavior of clang is to add -mllvm
--amdhsa-code-object-version=x only if users use -mcode-object-version=x or
equivalent explicitly. Otherwise clang should not add -mllvm
--amdhsa-code-object-version=x and let the backend decides the default code
object version.
@t-tye @kzhuravl what do you think? thanks.
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
