Skip to content

[CLANG][DRIVER] Resolve Argument Visibility and Duplication Issue for Cuda ToolChain #86807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[CLANG][DRIVER] Resolve Argument Visibility and Duplication Issue for Cuda ToolChain #86807

wants to merge 3 commits into from

Conversation

jle-quel
Copy link
Contributor

Description

This PR makes the argument -Xcuda-ptxas visible to the driver in cl-mode.

Furthermore, it has been noticed that the arguments are being passed twice to ptxas.
This also has been fixed by filtering out the arguments before appending them to the new DAL created by CudaToolChain::TranslateArgs.

@github-actions GitHub Actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Mar 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 27, 2024

@llvm/pr-subscribers-clang-driver

Author: Jefferson Le Quellec (jle-quel)

Changes

Description

This PR makes the argument -Xcuda-ptxas visible to the driver in cl-mode.

Furthermore, it has been noticed that the arguments are being passed twice to ptxas.
This also has been fixed by filtering out the arguments before appending them to the new DAL created by CudaToolChain::TranslateArgs.

Full diff: https://github.com/llvm/llvm-project/pull/86807.diff

3 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+2-1)
  • (modified) clang/lib/Driver/ToolChains/Cuda.cpp (+4-1)
  • (modified) clang/test/Driver/cuda-external-tools.cu (+8)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 29066ea14280c2..a5e132dc48a3ef 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1001,7 +1001,8 @@ def : Joined<["-"], "Xclang=">, Group<CompileOnly_Group>,
 def Xcuda_fatbinary : Separate<["-"], "Xcuda-fatbinary">,
   HelpText<"Pass <arg> to fatbinary invocation">, MetaVarName<"<arg>">;
 def Xcuda_ptxas : Separate<["-"], "Xcuda-ptxas">,
-  HelpText<"Pass <arg> to the ptxas assembler">, MetaVarName<"<arg>">;
+  HelpText<"Pass <arg> to the ptxas assembler">, MetaVarName<"<arg>">,
+  Visibility<[ClangOption, CLOption]>;
 def Xopenmp_target : Separate<["-"], "Xopenmp-target">, Group<CompileOnly_Group>,
   HelpText<"Pass <arg> to the target offloading toolchain.">, MetaVarName<"<arg>">;
 def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">, Group<CompileOnly_Group>,
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index 5f0b516e1a1a08..6634e6d818b33e 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -990,7 +990,10 @@ CudaToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
   }
 
   for (Arg *A : Args) {
-    DAL->append(A);
+    // Make sure flags are not duplicated.
+    if (!llvm::is_contained(*DAL, A)) {
+      DAL->append(A);
+    }
   }
 
   if (!BoundArch.empty()) {
diff --git a/clang/test/Driver/cuda-external-tools.cu b/clang/test/Driver/cuda-external-tools.cu
index 946e144fce38fb..d9564d026b4faa 100644
--- a/clang/test/Driver/cuda-external-tools.cu
+++ b/clang/test/Driver/cuda-external-tools.cu
@@ -86,6 +86,12 @@
 // RUN:   -Xcuda-fatbinary -bar1 -Xcuda-ptxas -foo2 -Xcuda-fatbinary -bar2 %s 2>&1 \
 // RUN: | FileCheck -check-prefixes=CHECK,SM35,PTXAS-EXTRA,FATBINARY-EXTRA %s
 
+// Check -Xcuda-ptxas with clang-cl
+// RUN: %clang_cl -### -c -Xcuda-ptxas -foo1 \
+// RUN:   --offload-arch=sm_35 --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
+// RUN:   -Xcuda-ptxas -foo2 %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=CHECK,SM35,PTXAS-EXTRA %s
+
 // MacOS spot-checks
 // RUN: %clang -### --target=x86_64-apple-macosx -O0 -c %s 2>&1 \
 // RUN:   --offload-arch=sm_35 --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
@@ -140,6 +146,8 @@
 // CHECK-SAME: "[[PTXFILE]]"
 // PTXAS-EXTRA-SAME: "-foo1"
 // PTXAS-EXTRA-SAME: "-foo2"
+// CHECK-NOT: "-foo1"
+// CHECK-NOT: "-foo2"
 // RDC-SAME: "-c"
 // CHECK-NOT: "-c"

@llvmbot
Copy link
Member

llvmbot commented Mar 27, 2024

@llvm/pr-subscribers-clang

Author: Jefferson Le Quellec (jle-quel)

Changes

Description

This PR makes the argument -Xcuda-ptxas visible to the driver in cl-mode.

Furthermore, it has been noticed that the arguments are being passed twice to ptxas.
This also has been fixed by filtering out the arguments before appending them to the new DAL created by CudaToolChain::TranslateArgs.

Full diff: https://github.com/llvm/llvm-project/pull/86807.diff

3 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+2-1)
  • (modified) clang/lib/Driver/ToolChains/Cuda.cpp (+4-1)
  • (modified) clang/test/Driver/cuda-external-tools.cu (+8)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 29066ea14280c2..a5e132dc48a3ef 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1001,7 +1001,8 @@ def : Joined<["-"], "Xclang=">, Group<CompileOnly_Group>,
 def Xcuda_fatbinary : Separate<["-"], "Xcuda-fatbinary">,
   HelpText<"Pass <arg> to fatbinary invocation">, MetaVarName<"<arg>">;
 def Xcuda_ptxas : Separate<["-"], "Xcuda-ptxas">,
-  HelpText<"Pass <arg> to the ptxas assembler">, MetaVarName<"<arg>">;
+  HelpText<"Pass <arg> to the ptxas assembler">, MetaVarName<"<arg>">,
+  Visibility<[ClangOption, CLOption]>;
 def Xopenmp_target : Separate<["-"], "Xopenmp-target">, Group<CompileOnly_Group>,
   HelpText<"Pass <arg> to the target offloading toolchain.">, MetaVarName<"<arg>">;
 def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">, Group<CompileOnly_Group>,
diff --git a/clang/lib/Driver/ToolChains/Cuda.cpp b/clang/lib/Driver/ToolChains/Cuda.cpp
index 5f0b516e1a1a08..6634e6d818b33e 100644
--- a/clang/lib/Driver/ToolChains/Cuda.cpp
+++ b/clang/lib/Driver/ToolChains/Cuda.cpp
@@ -990,7 +990,10 @@ CudaToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
   }
 
   for (Arg *A : Args) {
-    DAL->append(A);
+    // Make sure flags are not duplicated.
+    if (!llvm::is_contained(*DAL, A)) {
+      DAL->append(A);
+    }
   }
 
   if (!BoundArch.empty()) {
diff --git a/clang/test/Driver/cuda-external-tools.cu b/clang/test/Driver/cuda-external-tools.cu
index 946e144fce38fb..d9564d026b4faa 100644
--- a/clang/test/Driver/cuda-external-tools.cu
+++ b/clang/test/Driver/cuda-external-tools.cu
@@ -86,6 +86,12 @@
 // RUN:   -Xcuda-fatbinary -bar1 -Xcuda-ptxas -foo2 -Xcuda-fatbinary -bar2 %s 2>&1 \
 // RUN: | FileCheck -check-prefixes=CHECK,SM35,PTXAS-EXTRA,FATBINARY-EXTRA %s
 
+// Check -Xcuda-ptxas with clang-cl
+// RUN: %clang_cl -### -c -Xcuda-ptxas -foo1 \
+// RUN:   --offload-arch=sm_35 --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
+// RUN:   -Xcuda-ptxas -foo2 %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=CHECK,SM35,PTXAS-EXTRA %s
+
 // MacOS spot-checks
 // RUN: %clang -### --target=x86_64-apple-macosx -O0 -c %s 2>&1 \
 // RUN:   --offload-arch=sm_35 --cuda-path=%S/Inputs/CUDA/usr/local/cuda \
@@ -140,6 +146,8 @@
 // CHECK-SAME: "[[PTXFILE]]"
 // PTXAS-EXTRA-SAME: "-foo1"
 // PTXAS-EXTRA-SAME: "-foo2"
+// CHECK-NOT: "-foo1"
+// CHECK-NOT: "-foo2"
 // RDC-SAME: "-c"
 // CHECK-NOT: "-c"

@jle-quel jle-quel mentioned this pull request Mar 27, 2024
Closed
@jle-quel
Copy link
Contributor Author

jle-quel commented Apr 4, 2024

Gentle Ping to assign @rnk

ldrumm pushed a commit that referenced this pull request Apr 8, 2024
@jle-quel @ldrumm
Make the argument -Xcuda-ptxas visible to the driver in cl-mode
2921a09 
It has been noticed that the arguments are being passed twice to ptxas.
This also has been fixed by filtering out the arguments before appending
them to the new DAL created by CudaToolChain::TranslateArgs.

github:#86807
@ldrumm ldrumm closed this Apr 8, 2024
@jle-quel jle-quel deleted the jle-quel/update-clang-cl-driver branch November 7, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnk rnk rnk approved these changes

Assignees
No one assigned
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jle-quel @llvmbot @rnk @ldrumm