[PATCH] D40573: [NVPTX] Assign valid global names

[Jonas Hahnfeld via Phabricator via llvm-commits]

Hahnfeld added a comment.

This is essentially the fourth attempt after https://reviews.llvm.org/D17738, https://reviews.llvm.org/D29883, and https://reviews.llvm.org/D39005.

Let me come back to some of the previous comments:

In https://reviews.llvm.org/D17738#416060, @rnk wrote:

> The problem there is that we would have to make the IR symbol renamer not append '.N' when renaming symbols with name collisions. Until then, we need something like this.




In https://reviews.llvm.org/D17738#661115, @hfinkel wrote:

> It is also true that we need some way to generate unique symbol names that does not interfere with existing mangling schemes. This means that we need to have some character that is not in `[A-Za-z0-9_]`, and is not `@` (because that's used by ELF symbol versioning). `?$@'` are used by MSVC mangling. PTX allows only `[A-Za-z0-9_$]` (and also `%`, but only as the first character, so that doesn't help). Unfortunately, this does not seem to leave anything we can use (we can use '$' for PTX, but not for MSVC).
>
> One option is that we add a function to LLVM get an available separator character, which can default to '.', but we set to '$' for nvptx, and use that for generating new names at the IR level. Thoughts?


I opted to implement this approach as a special case in `ValueSymbolTable::makeUniqueName()`. That way we can't get an "invalid" result from `setName()` which avoids the loop that the previous patches used for cleanup.
I basically followed @hfinkel's analysis of which characters we can use which doesn't leave much choice.

In https://reviews.llvm.org/D17738#661168, @rnk wrote:

> This seems practical. Perhaps it could be part of the name mangling scheme already encoded in DataLayout?




In https://reviews.llvm.org/D39005#904580, @hfinkel wrote:

> DataLayout generally holds information that the target-independent optimizer needs in order to simplify the IR into our canonical form. This is as opposed to TargetTransformInfo, which provides data necessary to optimize the IR in target-aware ways (e.g., do things that are orthogonal to canonicalization such as inlining and vectorization). It is also as opposed to external utility functions that might be used by the frontend (e.g., llvm::sys::getHostCPUName()). If I recall correctly, this is information that would be used by the frontend when generating the IR, and the function results are controlled by the triple. As a result, I think that a general utility function somewhere would be fine.


I explicitly chose not to encode that information in the DataLayout:

1. I'm not sure the core IR library has access to the currently used DataLayout.
2. If I understood the code correctly, the DataLayout is set by the frontend, probably Clang in most cases. I don't think we should teach the frontend about a (correctness) decision that the backend can get right based on the selected target.


https://reviews.llvm.org/D40573