Partial revert "[HIP] Fix -mllvm option for device lld linker" #80202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This partially reverts commit aa964f1 because it caused perf regressions in rccl due to drop of -mllvm -amgpu-kernarg-preload-count=16 from the linker step. Pontentially it could cause similar regressions for other HIP apps using -mllvm options with -fgpu-rdc. Fixes: SWDEV-443345
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Yaxun (Sam) Liu (yxsamliu) ChangesThis partially reverts commit aa964f1 because it caused perf regressions in rccl due to drop of -mllvm -amgpu-kernarg-preload-count=16 from the linker step. Pontentially it could cause similar regressions for other HIP apps using -mllvm options with -fgpu-rdc. Fixes: SWDEV-443345 Full diff: https://github.com/llvm/llvm-project/pull/80202.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index ccb36a6c846c8..f9854a4840092 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -142,6 +142,11 @@ void AMDGCN::Linker::constructLldCommand(Compilation &C, const JobAction &JA,
if (IsThinLTO)
LldArgs.push_back(Args.MakeArgString("-plugin-opt=-force-import-all"));
+ for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+ LldArgs.push_back(
+ Args.MakeArgString(Twine("-plugin-opt=") + A->getValue(0)));
+ }
+
if (C.getDriver().isSaveTempsEnabled())
LldArgs.push_back("-save-temps");
diff --git a/clang/test/Driver/hip-toolchain-mllvm.hip b/clang/test/Driver/hip-toolchain-mllvm.hip
index 194cecc40db58..110d1c9b7fd33 100644
--- a/clang/test/Driver/hip-toolchain-mllvm.hip
+++ b/clang/test/Driver/hip-toolchain-mllvm.hip
@@ -1,9 +1,9 @@
// REQUIRES: x86-registered-target
// REQUIRES: amdgpu-registered-target
-// Check only -Xoffload-linker -mllvm=* options are passed
-// to device lld linker.
-// -mllvm options are passed to clang only.
+// Check -Xoffload-linker -mllvm=* options are passed
+// to device lld linker only.
+// -mllvm options are passed to clang and device lld linker.
// RUN: %clang -### --target=x86_64-linux-gnu \
// RUN: --offload-arch=gfx803 --offload-arch=gfx900 \
@@ -33,15 +33,15 @@
// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
// CHECK-SAME: {{.*}} "-target-cpu" "gfx803"
// CHECK-SAME: {{.*}} "-mllvm" "-unroll-count=10" {{.*}}
-// CHECK: [[LLD:".*lld.*"]] {{.*}}"-m" "elf64_amdgpu"{{.*}} "-plugin-opt=-inline-threshold=100"
+// CHECK: [[LLD:".*lld.*"]] {{.*}}"-m" "elf64_amdgpu"{{.*}} "-plugin-opt=-unroll-count=10"{{.*}} "-plugin-opt=-inline-threshold=100"
// CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
// CHECK-SAME: {{.*}} "-target-cpu" "gfx900"
// CHECK-SAME: {{.*}} "-mllvm" "-unroll-count=10" {{.*}}
-// CHECK: [[LLD:".*lld.*"]] {{.*}} "-plugin-opt=-inline-threshold=100"
+// CHECK: [[LLD:".*lld.*"]] {{.*}} "-plugin-opt=-unroll-count=10"{{.*}} "-plugin-opt=-inline-threshold=100"
// NEG-NOT: {{".*opt"}}
// NEG-NOT: {{".*llc"}}
-// NEG-NOT: "-plugin-opt=-unroll-count=10"
+// NEG-NOT: "-m" "elf_x86_64"{{.*}} "-plugin-opt=-unroll-count=10"
// NEG-NOT: "-m" "elf_x86_64"{{.*}} "-plugin-opt=-inline-threshold=100"
|
|
@llvm/pr-subscribers-clang-driver Author: Yaxun (Sam) Liu (yxsamliu) ChangesThis partially reverts commit aa964f1 because it caused perf regressions in rccl due to drop of -mllvm -amgpu-kernarg-preload-count=16 from the linker step. Pontentially it could cause similar regressions for other HIP apps using -mllvm options with -fgpu-rdc. Fixes: SWDEV-443345 Full diff: https://github.com/llvm/llvm-project/pull/80202.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index ccb36a6c846c8..f9854a4840092 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -142,6 +142,11 @@ void AMDGCN::Linker::constructLldCommand(Compilation &C, const JobAction &JA,
if (IsThinLTO)
LldArgs.push_back(Args.MakeArgString("-plugin-opt=-force-import-all"));
+ for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
+ LldArgs.push_back(
+ Args.MakeArgString(Twine("-plugin-opt=") + A->getValue(0)));
+ }
+
if (C.getDriver().isSaveTempsEnabled())
LldArgs.push_back("-save-temps");
diff --git a/clang/test/Driver/hip-toolchain-mllvm.hip b/clang/test/Driver/hip-toolchain-mllvm.hip
index 194cecc40db58..110d1c9b7fd33 100644
--- a/clang/test/Driver/hip-toolchain-mllvm.hip
+++ b/clang/test/Driver/hip-toolchain-mllvm.hip
@@ -1,9 +1,9 @@
// REQUIRES: x86-registered-target
// REQUIRES: amdgpu-registered-target
-// Check only -Xoffload-linker -mllvm=* options are passed
-// to device lld linker.
-// -mllvm options are passed to clang only.
+// Check -Xoffload-linker -mllvm=* options are passed
+// to device lld linker only.
+// -mllvm options are passed to clang and device lld linker.
// RUN: %clang -### --target=x86_64-linux-gnu \
// RUN: --offload-arch=gfx803 --offload-arch=gfx900 \
@@ -33,15 +33,15 @@
// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
// CHECK-SAME: {{.*}} "-target-cpu" "gfx803"
// CHECK-SAME: {{.*}} "-mllvm" "-unroll-count=10" {{.*}}
-// CHECK: [[LLD:".*lld.*"]] {{.*}}"-m" "elf64_amdgpu"{{.*}} "-plugin-opt=-inline-threshold=100"
+// CHECK: [[LLD:".*lld.*"]] {{.*}}"-m" "elf64_amdgpu"{{.*}} "-plugin-opt=-unroll-count=10"{{.*}} "-plugin-opt=-inline-threshold=100"
// CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
// CHECK-SAME: "-aux-triple" "x86_64-unknown-linux-gnu"
// CHECK-SAME: {{.*}} "-target-cpu" "gfx900"
// CHECK-SAME: {{.*}} "-mllvm" "-unroll-count=10" {{.*}}
-// CHECK: [[LLD:".*lld.*"]] {{.*}} "-plugin-opt=-inline-threshold=100"
+// CHECK: [[LLD:".*lld.*"]] {{.*}} "-plugin-opt=-unroll-count=10"{{.*}} "-plugin-opt=-inline-threshold=100"
// NEG-NOT: {{".*opt"}}
// NEG-NOT: {{".*llc"}}
-// NEG-NOT: "-plugin-opt=-unroll-count=10"
+// NEG-NOT: "-m" "elf_x86_64"{{.*}} "-plugin-opt=-unroll-count=10"
// NEG-NOT: "-m" "elf_x86_64"{{.*}} "-plugin-opt=-inline-threshold=100"
|
carlosgalvezp
pushed a commit
to carlosgalvezp/llvm-project
that referenced
this pull request
Feb 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This partially reverts commit aa964f1 because it caused perf regressions in rccl due to drop of -mllvm -amgpu-kernarg-preload-count=16 from the linker step. Potentially it could cause similar regressions for other HIP apps using -mllvm options with -fgpu-rdc.
Fixes: SWDEV-443345