[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

Wed May 25 15:00:22 PDT 2022

tra added a comment.

In D125904#3538163 <https://reviews.llvm.org/D125904#3538163>, @yaxunl wrote:

> I am OK with this patch.

OK.

> That said, this patch provided a default CUID that do not depend on driver, which is its advantage. It should be OK for most usecases.

I agree with this part.

> Driver provided CUID has more robustness, so it can serve as last resort.

I'd argue that it should be the default and, as such, the only mechanism for CUID generation. Doing it both in the driver and the codegen looks odd to me.

> If the fallback is not sufficient for the new driver then we can revisit this.

It would be great to have some compile-time checks for that, if possible. Otherwise it will only manifest at run-time and the end user will have no clue what's going on.

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6855
+      if (auto EC = llvm::sys::fs::getUniqueID(PLoc.getFilename(), ID))
+        SM.getDiagnostics().Report(diag::err_cannot_open_file)
+            << PLoc.getFilename() << EC.message();
----------------
If we're compiling an already preprocessed file with `#line` directives, I assume that the source location will be pointing to the file specified by `#line`, not the input file itself.
If that file is not available (and it does not have to. E.g. I someone has preprocess the original CUDA source and use creduce on the preprocessed code) then we'll get an error when we should not have.

It also results in a different CUIDs being generated for different identifiers, which is different from one CUID for everything generated during the compilation. I can not tell whether it matters in practice. As long as they are in sync betwee the host and the device, it should be OK and it may even have the benefit of allowing commoning things that come from headers, while driver-set common CUID would make all such instances unique.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125904/new/

https://reviews.llvm.org/D125904