[clang] a446827 - [NFC][Clang][Driver][AMDGPU] Avoid temporary copies of std::string by using Twine and StringRef

[David Blaikie via cfe-commits]

On Mon, Dec 5, 2022 at 5:27 AM Juan Manuel MARTINEZ CAAMAÑO via
cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
>
> Author: Juan Manuel MARTINEZ CAAMAÑO
> Date: 2022-12-05T07:27:10-06:00
> New Revision: a446827249bdeb2f27e55a9f4942bd7425ecb0ff
>
> URL: https://github.com/llvm/llvm-project/commit/a446827249bdeb2f27e55a9f4942bd7425ecb0ff
> DIFF: https://github.com/llvm/llvm-project/commit/a446827249bdeb2f27e55a9f4942bd7425ecb0ff.diff
>
> LOG: [NFC][Clang][Driver][AMDGPU] Avoid temporary copies of std::string by using Twine and StringRef
>
> Reviewed By: tra
>
> Differential Revision: https://reviews.llvm.org/D139023
>
> Added:
>
>
> Modified:
>     clang/lib/Driver/ToolChains/AMDGPU.cpp
>     clang/lib/Driver/ToolChains/HIPAMD.cpp
>     clang/lib/Driver/ToolChains/ROCm.h
>
> Removed:
>
>
>
> ################################################################################
> diff  --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp
> index f3ac7d2ab735..ff77969bbec5 100644
> --- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
> +++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
> @@ -854,7 +854,7 @@ void ROCMToolChain::addClangTargetOptions(
>    const StringRef GpuArch = getGPUArch(DriverArgs);
>    auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch);
>    const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
> -  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
> +  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
>    auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
>        getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
>    if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
> @@ -946,7 +946,7 @@ ROCMToolChain::getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,
>    auto Kind = llvm::AMDGPU::parseArchAMDGCN(GPUArch);
>    const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
>
> -  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
> +  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
>    auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
>        getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
>    if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
>
> diff  --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
> index c64421d259ce..a555fe5830e0 100644
> --- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
> +++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
> @@ -237,7 +237,7 @@ void HIPAMDToolChain::addClangTargetOptions(
>        DriverArgs.getLastArgValue(options::OPT_gpu_max_threads_per_block_EQ);
>    if (!MaxThreadsPerBlock.empty()) {
>      std::string ArgStr =
> -        std::string("--gpu-max-threads-per-block=") + MaxThreadsPerBlock.str();
> +        (Twine("--gpu-max-threads-per-block=") + MaxThreadsPerBlock).str();
>      CC1Args.push_back(DriverArgs.MakeArgStringRef(ArgStr));
>    }
>
> @@ -344,7 +344,7 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
>    ArgStringList LibraryPaths;
>
>    // Find in --hip-device-lib-path and HIP_LIBRARY_PATH.
> -  for (auto Path : RocmInstallation.getRocmDeviceLibPathArg())
> +  for (StringRef Path : RocmInstallation.getRocmDeviceLibPathArg())
>      LibraryPaths.push_back(DriverArgs.MakeArgString(Path));
>
>    addDirectoryList(DriverArgs, LibraryPaths, "", "HIP_DEVICE_LIB_PATH");
> @@ -354,7 +354,7 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
>    if (!BCLibArgs.empty()) {
>      llvm::for_each(BCLibArgs, [&](StringRef BCName) {
>        StringRef FullName;
> -      for (std::string LibraryPath : LibraryPaths) {
> +      for (StringRef LibraryPath : LibraryPaths) {
>          SmallString<128> Path(LibraryPath);
>          llvm::sys::path::append(Path, BCName);
>          FullName = Path;
> @@ -387,15 +387,15 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
>          getDriver().Diag(DiagID);
>          return {};
>        } else
> -        BCLibs.push_back({AsanRTL.str(), /*ShouldInternalize=*/false});
> +        BCLibs.emplace_back(AsanRTL, /*ShouldInternalize=*/false);
>      }
>
>      // Add the HIP specific bitcode library.
>      BCLibs.push_back(RocmInstallation.getHIPPath());
>
>      // Add common device libraries like ocml etc.
> -    for (auto N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
> -      BCLibs.push_back(StringRef(N));
> +    for (StringRef N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
> +      BCLibs.emplace_back(N);
>
>      // Add instrument lib.
>      auto InstLib =
>
> diff  --git a/clang/lib/Driver/ToolChains/ROCm.h b/clang/lib/Driver/ToolChains/ROCm.h
> index b16deecdebec..600c8b39f4b3 100644
> --- a/clang/lib/Driver/ToolChains/ROCm.h
> +++ b/clang/lib/Driver/ToolChains/ROCm.h
> @@ -252,8 +252,11 @@ class RocmInstallationDetector {
>    }
>
>    /// Get libdevice file for given architecture
> -  std::string getLibDeviceFile(StringRef Gpu) const {
> -    return LibDeviceMap.lookup(Gpu);
> +  StringRef getLibDeviceFile(StringRef Gpu) const {
> +    auto Loc = LibDeviceMap.find(Gpu);
> +    if (Loc == LibDeviceMap.end())
> +      return "";
> +    return Loc->second;
>    }
>
>    void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
> @@ -263,7 +266,7 @@ class RocmInstallationDetector {
>    void detectHIPRuntime();
>
>    /// Get the values for --rocm-device-lib-path arguments
> -  std::vector<std::string> getRocmDeviceLibPathArg() const {
> +  const ArrayRef<std::string> getRocmDeviceLibPathArg() const {

Generally return values shouldn't be `const` - since there's a copy
being made here anyway. Can you remove the `const`?

>      return RocmDeviceLibPathArg;
>    }
>
> @@ -273,7 +276,7 @@ class RocmInstallationDetector {
>    /// Get the value for --hip-version argument
>    StringRef getHIPVersionArg() const { return HIPVersionArg; }
>
> -  std::string getHIPVersion() const { return DetectedVersion; }
> +  StringRef getHIPVersion() const { return DetectedVersion; }
>  };
>
>  } // end namespace driver
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits