[PATCH] D45212: Add HIP toolchain

[Artem Belevich via Phabricator via cfe-commits]

tra 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);
----------------
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.



================
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";
----------------
No need for intermediate values here -- just '+' all parts together. 



================
Comment at: lib/Driver/ToolChains/HIP.cpp:133
+    }
+    OptArgs.push_back(Args.MakeArgString(llvm::Twine("-O") + OOpt));
+  }
----------------
Nit: I think explicit llvm::Twine is unnecessary here. 


================
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");
----------------
Nit: THis could be collapsed into `ArgStringList LlcArgs({...});`


================
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"});
----------------
Same here: `ArgStringList LldArgs({"-flavor", "gnu", "--no-undefined", "-shared", "-o"});`


================
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);
----------------
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.


https://reviews.llvm.org/D45212