[PATCH] D45212: Add HIP toolchain

[Yaxun Liu via Phabricator via cfe-commits]

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