yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
================
Comment at: lib/Driver/ToolChains/HIP.cpp:29-47
+static bool addBCLib(Compilation &C, const ArgList &Args,
+ ArgStringList &CmdArgs, ArgStringList LibraryPaths,
+ StringRef BCName) {
+ StringRef FullName;
+ bool FoundLibDevice = false;
+ for (std::string LibraryPath : LibraryPaths) {
+ SmallString<128> Path(LibraryPath);
----------------
tra wrote:
> FullName may remain uninitialized if LibraryPaths are empty which will
> probably crash compiler when you attempt to pass it to MakeArgString.
> If empty LibraryPaths is not expected there should be an assert.
>
> If the library is not found, we issue an error, but we still proceed to
> append the FullName to the CmdArgs. I don't think we should do that. FullName
> will be either NULL or pointing to the last directory in the LibraryPaths.
>
> You seem to be relying on diagnostics to deal with errors and are not using
> return value of the function. You may as well make it void.
>
> I'd move `CmdArgs.push_back(...)` under `if(::exists(FullName))` and change
> `break` to `return`;
> Then you can get rid of FoundLibDevice and just issue the error if we ever
> reach the end of the function.
>
Will CmdArgs.push_back(...) under if(::exists(FullName)) and change break to
return; and change return type to void.
================
Comment at: lib/Driver/ToolChains/HIP.cpp:79-81
+ std::string ISAVerBC = "oclc_isa_version_";
+ ISAVerBC = ISAVerBC + SubArchName.drop_front(3).str();
+ ISAVerBC = ISAVerBC + ".amdgcn.bc";
----------------
tra wrote:
> No need for intermediate values here -- just '+' all parts together.
>
will do
================
Comment at: lib/Driver/ToolChains/HIP.cpp:133
+ }
+ OptArgs.push_back(Args.MakeArgString(llvm::Twine("-O") + OOpt));
+ }
----------------
tra wrote:
> Nit: I think explicit llvm::Twine is unnecessary here.
will remove
================
Comment at: lib/Driver/ToolChains/HIP.cpp:155-160
+ ArgStringList LlcArgs;
+ LlcArgs.push_back(InputFileName);
+ LlcArgs.push_back("-mtriple=amdgcn-amd-amdhsa");
+ LlcArgs.push_back("-filetype=obj");
+ LlcArgs.push_back(Args.MakeArgString("-mcpu=" + SubArchName));
+ LlcArgs.push_back("-o");
----------------
tra wrote:
> Nit: THis could be collapsed into `ArgStringList LlcArgs({...});`
will do
================
Comment at: lib/Driver/ToolChains/HIP.cpp:179-181
+ ArgStringList LldArgs;
+ // The output from ld.lld is an HSA code object file.
+ LldArgs.append({"-flavor", "gnu", "--no-undefined", "-shared", "-o"});
----------------
tra wrote:
> Same here: `ArgStringList LldArgs({"-flavor", "gnu", "--no-undefined",
> "-shared", "-o"});`
will do
================
Comment at: lib/Driver/ToolChains/HIP.cpp:212-215
+ TempFile =
+ constructOptCommand(C, JA, Inputs, Args, SubArchName, Prefix, TempFile);
+ TempFile =
+ constructLlcCommand(C, JA, Inputs, Args, SubArchName, Prefix, TempFile);
----------------
tra wrote:
> Right now the code is structured as if you're appending to the same TempFile
> string which is not the case here. I'd give intermediate variables their own
> names -- `OptCommand`,`LlcCommand`.
> This would make it easier to see that you are **chaining** separate commands,
> each producing its own temp output file.
will do
https://reviews.llvm.org/D45212
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
