[PATCH] D17738: [NVPTX] Fix function identifiers that are invalid in PTX and a bug fix for the case of name collisions.

[Rafael Espíndola via llvm-commits]

Wouldn't it be better to error and ask the llvm producer to not create
global values with '@' and '.' when targeting nvptx?

Cheers,
Rafael


On 29 February 2016 at 16:55, Arpith Jacob <acjacob at us.ibm.com> wrote:
> arpith-jacob created this revision.
> arpith-jacob added reviewers: eliben, rafael.
> arpith-jacob added subscribers: jlebar, jholewinski, caomhin, carlo.bertolli, sfantao, llvm-commits.
>
> The characters '.' and '@' are not allowed by PTX in identifiers.  A [[ http://reviews.llvm.org/rL205212 | previous patch ]] cleans up such identifiers for global variables.  This patch cleans up identifiers of local functions as well.
>
> An [[ http://reviews.llvm.org/rL253804 | unrelated patch ]] changed the name collision avoidance code in LLVM by adding a suffix of the pattern ["." number].  This broke NVPTXAssignValidGlobalNames.cpp as the dot is introduced in the case of collisions.  I fix this by cleaning up the new name and repeating as required.
>
> http://reviews.llvm.org/D17738
>
> Files:
>   lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
>   test/CodeGen/NVPTX/symbol-naming.ll
>
> Index: test/CodeGen/NVPTX/symbol-naming.ll
> ===================================================================
> --- test/CodeGen/NVPTX/symbol-naming.ll
> +++ test/CodeGen/NVPTX/symbol-naming.ll
> @@ -3,14 +3,15 @@
>
>  ; Verify that the NVPTX target removes invalid symbol names prior to emitting
>  ; PTX.
> +; A symbol is invalid in PTX if it contains a '.' or a '@'.
>
>  ; PTX32-NOT: .str
>  ; PTX64-NOT: .str
>
> -; PTX32-DAG: _$_str.1
> +; PTX32-DAG: _$_str_$_1
>  ; PTX32-DAG: _$_str
>
> -; PTX64-DAG: _$_str.1
> +; PTX64-DAG: _$_str_$_1
>  ; PTX64-DAG: _$_str
>
>  target datalayout = "e-i64:64-v16:16-v32:32-n16:32:64"
> @@ -28,4 +29,15 @@
>    ret void
>  }
>
> +; PTX32-NOT: omp_outlined.1
> +; PTX64-NOT: omp_outlined.1
> +; PTX32-DAG: omp_outlined_$_1_$_2_$_3
> +; PTX64-DAG: omp_outlined_$_1_$_2_$_3
> +define internal void @omp_outlined.1() {
> +  ret void
> +}
> +
> +declare void @omp_outlined_$_1()
> +declare void @omp_outlined_$_1_$_2()
> +
>  declare i32 @printf(i8*, ...)
> Index: lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
> ===================================================================
> --- lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
> +++ lib/Target/NVPTX/NVPTXAssignValidGlobalNames.cpp
> @@ -37,6 +37,8 @@
>
>    /// \brief Clean up the name to remove symbols invalid in PTX.
>    std::string cleanUpName(StringRef Name);
> +  /// \brief Set a clean name ensuring collisions are avoided.
> +  void generateCleanName(Value &V);
>  };
>  }
>
> @@ -53,17 +55,33 @@
>    for (GlobalVariable &GV : M.globals()) {
>      // We are only allowed to rename local symbols.
>      if (GV.hasLocalLinkage()) {
> -      // setName doesn't do extra work if the name does not change.
> -      // Note: this does not create collisions - if setName is asked to set the
> -      // name to something that already exists, it adds a proper postfix to
> -      // avoid collisions.
> -      GV.setName(cleanUpName(GV.getName()));
> +      generateCleanName(GV);
> +    }
> +  }
> +
> +  // Clean function symbols.
> +  for (auto &FN : M.functions()) {
> +    if (FN.hasLocalLinkage()) {
> +      generateCleanName(FN);
>      }
>    }
>
>    return true;
>  }
>
> +void NVPTXAssignValidGlobalNames::generateCleanName(Value &V) {
> +  while (1) {
> +    StringRef ValidName = cleanUpName(V.getName());
> +    // setName doesn't do extra work if the name does not change.
> +    // Collisions are avoided by adding a suffix (which may yet be unclean in
> +    // PTX).
> +    V.setName(ValidName);
> +    // If there are no collisions return, otherwise clean up the new name.
> +    if (V.getName() == ValidName)
> +      return;
> +  }
> +}
> +
>  std::string NVPTXAssignValidGlobalNames::cleanUpName(StringRef Name) {
>    std::string ValidName;
>    raw_string_ostream ValidNameStream(ValidName);
>
>