Hahnfeld added a comment.
Let's start with the obvious points:
1. The latest patch clearly wasn't run through `clang-format`.
2. It only has some 90 lines of context which makes it look like you deleted
quite some code when browsing through the history.
3. This patch is still large, did you at least consider splitting it as I
proposed some weeks ago?
Additionally I don't see responses to all points that @tra raised. Their
answers will probably explain the approach you chose, so I think they should
also be added to the summary.
================
Comment at: lib/Basic/Cuda.cpp:290-302
+ case CudaArch::GFX701:
+ case CudaArch::GFX702:
+ case CudaArch::GFX703:
+ case CudaArch::GFX704:
+ case CudaArch::GFX800:
+ case CudaArch::GFX801:
+ case CudaArch::GFX802:
----------------
This means you can't compile when the Clang driver detected an installation of
CUDA 9?
================
Comment at: lib/Basic/Targets/AMDGPU.h:383
uint64_t getNullPointerValue(LangAS AS) const override {
+ if(getTriple().getOS()==llvm::Triple::CUDA) return 0;
return AS == LangAS::opencl_local ? ~0 : 0;
----------------
How can `LangAS::opencl_local` ever be used in CUDA? I think this check is
redundant.
================
Comment at: lib/Driver/Driver.cpp:3262-3263
!C.getArgs().hasArg(options::OPT_traditional_cpp) && !SaveTemps &&
+ (InputType != types::TY_LLVM_IR) &&
+ (InputType != types::TY_LLVM_BC) &&
!C.getArgs().hasArg(options::OPT_rewrite_objc);
----------------
I'm not sure I understand this change to the generic driver code: How can LLVM
IR / BC ever need a preprocessor?
================
Comment at: lib/Driver/Driver.cpp:3954-3957
+ if (((StringRef)BaseInput).endswith(".a"))
+ Suffixed += "a";
+ else
+ Suffixed += Suffix;
----------------
If this is really needed this deserves some justification.
================
Comment at: lib/Driver/ToolChains/Cuda.cpp:359-361
+ addBCLib(C, Args, CmdArgs, LibraryPaths, "opencl.amdgcn.bc");
+ addBCLib(C, Args, CmdArgs, LibraryPaths, "ockl.amdgcn.bc");
+ addBCLib(C, Args, CmdArgs, LibraryPaths, "irif.amdgcn.bc");
----------------
gregrodgers wrote:
> arsenm wrote:
> > Why is this done under an NVPTX:: class
> Because we are not creating a new toolchain for AMDGCN. We modify the logic
> in the tool constructor as needed for AMDGCN, keeping the ability to provide
> a set of mixed targets.
That sounds more like a hack, the CUDA classes should be separated from NVPTX
and AMDGCN.
================
Comment at: lib/Driver/ToolChains/Cuda.cpp:639-640
+ CmdArgs2.push_back(Args.MakeArgString(Output.getFilename()));
+ const char *Exec2 =
+ Args.MakeArgString(C.getDriver().Dir + "/clang-fixup-fatbin");
+ C.addCommand(
----------------
gregrodgers wrote:
> Hahnfeld wrote:
> > `clang-fixup-fatbin` is not upstreamed and won't work. Sounds like a
> > horrible name btw...
> Major cleanup here in the next update. It is not a bad name when you see the
> update and the comments in the update.
I wasn't only criticizing the name but also the fact that this code won't work
with only upstream components!
================
Comment at: lib/Driver/ToolChains/Cuda.cpp:293
+ if (useOpenHeaders()) {
+ CC1Args.push_back("__clang_cuda_runtime_wrapper_open.h");
+ CC1Args.push_back("-D__USE_OPEN_HEADERS__");
----------------
I don't see this file in the upstream repository?
================
Comment at: lib/Driver/ToolChains/Cuda.cpp:413
+
+ addEnvListWithSpaces(Args, CmdArgs, "CLANG_TARGET_LINK_OPTS");
+ CmdArgs.push_back("-suppress-warnings");
----------------
I don't think you should invent new environment variables, Clang normally uses
the `-X` class to pass arguments to specific parts of the toolchain.
https://reviews.llvm.org/D42800
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
