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)));
----------------
yaxunl wrote:
> 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.
That sounds right to me. It's slightly more complicated to implement than this,
will need to rework getOrCheckAMDGPUCodeObjectVersion to distinguish between
the get and check parts.
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
