Skip to content

clang: Remove requires system-linux from some driver tests #111976

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.

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

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Oct 11, 2024

Works for me on macos.

@arsenm arsenm added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Oct 11, 2024 — with Graphite App
@arsenm arsenm marked this pull request as ready for review October 11, 2024 10:46
@llvmbot
Copy link
Member

llvmbot commented Oct 11, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Matt Arsenault (arsenm)

Changes

Works for me on macos.


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

7 Files Affected:

  • (modified) clang/test/Driver/amdgpu-hip-system-arch.c (-1)
  • (modified) clang/test/Driver/amdgpu-openmp-system-arch-fail.c (-1)
  • (modified) clang/test/Driver/hip-partial-link.hip (+1-1)
  • (modified) clang/test/Driver/hip-temps-linux.hip (-1)
  • (modified) clang/test/Driver/linker-wrapper.c (+2-4)
  • (modified) clang/test/Driver/nvptx-cuda-system-arch.c (-1)
  • (modified) clang/test/Driver/openmp-system-arch.c (-1)
diff --git a/clang/test/Driver/amdgpu-hip-system-arch.c b/clang/test/Driver/amdgpu-hip-system-arch.c
index f25a4087080f6d..7be7b9cad1be02 100644
--- a/clang/test/Driver/amdgpu-hip-system-arch.c
+++ b/clang/test/Driver/amdgpu-hip-system-arch.c
@@ -1,4 +1,3 @@
-// REQUIRES: system-linux
 // REQUIRES: shell
 
 // RUN: mkdir -p %t
diff --git a/clang/test/Driver/amdgpu-openmp-system-arch-fail.c b/clang/test/Driver/amdgpu-openmp-system-arch-fail.c
index 85c82e4598cb10..b7e1d0b2c56659 100644
--- a/clang/test/Driver/amdgpu-openmp-system-arch-fail.c
+++ b/clang/test/Driver/amdgpu-openmp-system-arch-fail.c
@@ -1,4 +1,3 @@
-// REQUIRES: system-linux
 // REQUIRES: shell
 
 // RUN: mkdir -p %t
diff --git a/clang/test/Driver/hip-partial-link.hip b/clang/test/Driver/hip-partial-link.hip
index c8451ec81ed37e..0bb8a848f80718 100644
--- a/clang/test/Driver/hip-partial-link.hip
+++ b/clang/test/Driver/hip-partial-link.hip
@@ -1,4 +1,4 @@
-// REQUIRES: x86-registered-target, amdgpu-registered-target, lld, system-linux
+// REQUIRES: x86-registered-target, amdgpu-registered-target, lld
 
 // RUN: %clang -x hip --target=x86_64-unknown-linux-gnu --no-offload-new-driver \
 // RUN:   --offload-arch=gfx906 -c -nostdinc -nogpuinc -nohipwrapperinc \
diff --git a/clang/test/Driver/hip-temps-linux.hip b/clang/test/Driver/hip-temps-linux.hip
index 83a7528dd4560a..3fb8a94a3463c8 100644
--- a/clang/test/Driver/hip-temps-linux.hip
+++ b/clang/test/Driver/hip-temps-linux.hip
@@ -1,6 +1,5 @@
 // REQUIRES: x86-registered-target
 // REQUIRES: amdgpu-registered-target
-// REQUIRES: system-linux
 
 // Check no temporary files or directores are left after compilation.
 // RUN: rm -rf %t/mytmp
diff --git a/clang/test/Driver/linker-wrapper.c b/clang/test/Driver/linker-wrapper.c
index 068ea2d7d3c663..976f7c170fdd69 100644
--- a/clang/test/Driver/linker-wrapper.c
+++ b/clang/test/Driver/linker-wrapper.c
@@ -2,8 +2,6 @@
 // REQUIRES: nvptx-registered-target
 // REQUIRES: amdgpu-registered-target
 
-// REQUIRES: system-linux
-
 // An externally visible variable so static libraries extract.
 __attribute__((visibility("protected"), used)) int x;
 
@@ -30,7 +28,7 @@ __attribute__((visibility("protected"), used)) int x;
 // RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run --device-debug -O0 \
 // RUN:   --linker-path=/usr/bin/ld %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=NVPTX-LINK-DEBUG
 
-// NVPTX-LINK-DEBUG: clang{{.*}} -o {{.*}}.img --target=nvptx64-nvidia-cuda -march=sm_70 -O2 -flto {{.*}}.o {{.*}}.o -g 
+// NVPTX-LINK-DEBUG: clang{{.*}} -o {{.*}}.img --target=nvptx64-nvidia-cuda -march=sm_70 -O2 -flto {{.*}}.o {{.*}}.o -g
 
 // RUN: clang-offload-packager -o %t.out \
 // RUN:   --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908 \
@@ -93,7 +91,7 @@ __attribute__((visibility("protected"), used)) int x;
 
 // CUDA: clang{{.*}} -o [[IMG_SM70:.+]] --target=nvptx64-nvidia-cuda -march=sm_70
 // CUDA: clang{{.*}} -o [[IMG_SM52:.+]] --target=nvptx64-nvidia-cuda -march=sm_52
-// CUDA: fatbinary{{.*}}-64 --create {{.*}}.fatbin --image=profile=sm_70,file=[[IMG_SM70]] --image=profile=sm_52,file=[[IMG_SM52]] 
+// CUDA: fatbinary{{.*}}-64 --create {{.*}}.fatbin --image=profile=sm_70,file=[[IMG_SM70]] --image=profile=sm_52,file=[[IMG_SM52]]
 // CUDA: usr/bin/ld{{.*}} {{.*}}.openmp.image.{{.*}}.o {{.*}}.cuda.image.{{.*}}.o
 
 // RUN: clang-offload-packager -o %t.out \
diff --git a/clang/test/Driver/nvptx-cuda-system-arch.c b/clang/test/Driver/nvptx-cuda-system-arch.c
index 6a8a218406d139..b6a7617930fc19 100644
--- a/clang/test/Driver/nvptx-cuda-system-arch.c
+++ b/clang/test/Driver/nvptx-cuda-system-arch.c
@@ -1,4 +1,3 @@
-// REQUIRES: system-linux
 // REQUIRES: shell
 
 // RUN: mkdir -p %t
diff --git a/clang/test/Driver/openmp-system-arch.c b/clang/test/Driver/openmp-system-arch.c
index cd49f460099666..75322dae69de46 100644
--- a/clang/test/Driver/openmp-system-arch.c
+++ b/clang/test/Driver/openmp-system-arch.c
@@ -1,4 +1,3 @@
-// REQUIRES: system-linux
 // REQUIRES: shell
 
 // RUN: mkdir -p %t

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AMDGPU labels Oct 11, 2024
Copy link
Contributor

@perry-ca perry-ca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three of these tests will fail on z/OS when the requires is removed. It would be great if they can be fixed so they work on z/OS. None of these are available or supported on z/OS so marking them as unsupported on system-zos is also an option.

@daltenty do you want to check AIX too?
Thanks.

@@ -1,4 +1,3 @@
// REQUIRES: system-linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compile & test on line 47 is failing on z/OS with the following message:

clang: error: cannot determine amdgcn architecture: No AMD GPU detected in the system; consider passing it via '--offload-arch'; environment variable CLANG_TOOLCHAIN_PROGRAM_TIMEOUT specifies the tool timeout (integer secs, <=0 is infinite)

I'm not sure if there is a way to fix this (eg. wildcard in the expected output or a compiler option). If there isn't can you add:

UNSUPPORTED: system-zos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't really be a property of zos or any other system. It works for me on systems without AMDGPUs and other OSes. You are seeing the error after the environment variable parse error, so why did this proceed beyond that point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should have been line 26. The error is causing the failure. The env var isn't set for that compilation.

@@ -1,4 +1,3 @@
// REQUIRES: system-linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing on z/OS on line 28 (ARCH_sm70 check). The actual cc1 line is:

clang: warning: CUDA version is newer than the latest partially supported version 12.5 [-Wunknown-cuda-version]
clang: error: cannot determine nvptx64 architecture: No NVIDIA GPU detected in the system; consider passing it via '--offload-arch'; environment variable CLANG_TOOLCHAIN_PROGRAM_TIMEOUT specifies the tool timeout (integer secs, <=0 is infinite)
 "/plex/perry/llvm/Woz/build/bin/clang-20" "-cc1" "-triple" "nvptx64-nvidia-cuda" "-aux-triple" "x86_64-unknown-linux-gnu" "-S" "-dumpdir" "a-" "-disable-free" "-clear-ast-before-backend" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "nvptx-cuda-system-arch.c" "-mrelocation-model" "static" "-mframe-pointer=all" "-fno-rounding-math" "-no-integrated-as" "-aux-target-cpu" "x86-64" "-fcuda-is-device" "-mllvm" "-enable-memcpyopt-without-libcalls" "-fcuda-allow-variadic-functions" "-target-cpu" "sm_52" "-target-feature" "+ptx85" "-debugger-tuning=gdb" "-fno-dwarf-directory-asm" "-fdebug-compilation-dir=/plex/perry/llvm/Woz/llvm-project/clang/test/Driver" "-resource-dir" "/plex/perry/llvm/Woz/build/lib/clang/20" "-internal-isystem" "/plex/perry/llvm/Woz/build/lib/clang/20/include/cuda_wrappers" "-include" "__clang_cuda_runtime_wrapper.h" "-internal-isystem" "/plex/perry/llvm/Woz/build/lib/clang/20/include" "-internal-isystem" "/usr/local/include" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-internal-isystem" "/plex/perry/llvm/Woz/llvm-project/clang/test/Driver/Inputs/CUDA_102/usr/local/cuda/include" "-internal-isystem" "/plex/perry/llvm/Woz/build/lib/clang/20/include" "-internal-isystem" "/usr/local/include" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-fdeprecated-macro" "-fno-autolink" "-ferror-limit" "19" "-nogpulib" "-fgnuc-version=4.2.1" "-mno-csect" "-fskip-odr-check-in-gmf" "-fcxx-exceptions" "-fexceptions" "-fexec-charset" "UTF-8" "-fcolor-diagnostics" "-cuid=15151f95f8ae4b17" "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" "/plex/perry/tmp/nvptx-cuda-system-arch-sm_52-77f5c9.s" "-x" "cuda" "/plex/perry/llvm/Woz/llvm-project/clang/test/Driver/nvptx-cuda-system-arch.c"

This has sm_52 as the target_gpu instead of sm_70. Can you either fix it or add:

UNSUPPORTED: system-zos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result should definitely be sm_70, that's what the provided script produces

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. By fixing it, I was referring to the code in clang that determines this value. If it's easy to make the test pass on z/OS great. If not marking it as unsupported works too. This applies to my other comment too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the output from a shell script in clang/test/Driver/Inputs/nvptx-arch/nvptx_arch_sm_70

How could the zos test find a different result

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking into that question. The first thing I noticed was the temp file is being created and read as a binary file. I also noticed that the output of the script is not being redirected into the temp file. I'll get a PR up to fix these problems once I get it solved.

I think that mean the only issue on z/OS is not related to these tests. Could you mark the 3 failing tests as XFAIL for system-zos. I'll remove that once I have the problem fixed.

@@ -1,4 +1,3 @@
// REQUIRES: system-linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing on z/OS on line 33 with the error:

clang: error: failed to deduce triple for target architecture 'native'; specify the triple using '-fopenmp-targets' and '-Xopenmp-target' instead

Not deducing the target gpu causes wrong -target-cpu to be specified.

If it can be fixed great. If not can you add:

UNSUPPORTED: system-zos

Base automatically changed from users/arsenm/hipstdpar-fix-host-dependent-test to main October 15, 2024 05:45
@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch from fee09ae to b63c440 Compare October 15, 2024 18:03
Copy link
Contributor

@perry-ca perry-ca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Matt.

@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch 3 times, most recently from 0d43cba to e2dfde6 Compare October 18, 2024 05:48
@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch from e2dfde6 to c73e500 Compare October 28, 2024 22:31
perry-ca added a commit that referenced this pull request Oct 30, 2024
…4067)

PR #111976 was enabling the
tests updated in the PR to run on all systems. We found a few didn't run
on z/OS. I tracked the problem down to:
1. the ExecuteToolChainProgram() function wasn't passing the executable
name as the first arg. That was causing exec on z/OS to fail.
2. the temp file needs to be a text file so codepage conversion happens.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…m#114067)

PR llvm#111976 was enabling the
tests updated in the PR to run on all systems. We found a few didn't run
on z/OS. I tracked the problem down to:
1. the ExecuteToolChainProgram() function wasn't passing the executable
name as the first arg. That was causing exec on z/OS to fail.
2. the temp file needs to be a text file so codepage conversion happens.
@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch 3 times, most recently from 8c678b6 to e26a8f2 Compare November 5, 2024 21:37
@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch from e26a8f2 to 655a5fb Compare November 13, 2024 18:29
@arsenm arsenm force-pushed the users/arsenm/hip-partial-link-remove-requires-linux branch from 655a5fb to 99da25f Compare November 14, 2024 00:30
@arsenm arsenm merged commit 03730cd into main Nov 14, 2024
8 checks passed
@arsenm arsenm deleted the users/arsenm/hip-partial-link-remove-requires-linux branch November 14, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants