Skip to content
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

[CI] Temporarily disable tests requiring spirv-tools in CI #16743

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Jan 23, 2025

#16724 installed spirv-tools on Linux docker containers and now some llvm-spirv tests are failing due to this (example: https://github.com/intel/llvm/actions/runs/12915358763/job/36017038536).
In this PR, we disable detection of spirv-tools LIT feature temporarily in CI to fix post-commit.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Approving to unblock fixing CI failures, but llvm-spirv changes should either be upstreamed, or tracked in #7592 to remind us to revert them

@maarquitos14
Copy link
Contributor

I'm okay with merging this to fix post-commit.

About the root cause, the problem seems to be that the path for all spirv-tools binaries is not found:

llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-as in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-dis in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:[12](https://github.com/intel/llvm/actions/runs/12918158681/job/36026109985#step:14:13)6: note: Did not find spirv-link in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-val in SPIRV_AS_PATH-NOTFOUND

I'm not sure why, I cannot reproduce locally. If you help me reproduce the issue, I can try and fix it.

Co-authored-by: Marcos Maronas <[email protected]>
@uditagarwal97 uditagarwal97 changed the title [CI] Temporarily disable tests requiring spriv-tools in CI [CI] Temporarily disable tests requiring spirv-tools in CI Jan 23, 2025
@uditagarwal97
Copy link
Contributor Author

@intel/llvm-gatekeepers I think we are good to merge this.

@sarnex sarnex merged commit f12546b into sycl Jan 23, 2025
22 of 30 checks passed
@aelovikov-intel aelovikov-intel deleted the sycl-devops-pr/udit/llvm_spirv branch January 23, 2025 18:52
@uditagarwal97
Copy link
Contributor Author

uditagarwal97 commented Jan 25, 2025

Hi @maarquitos14, @intel/dpcpp-spirv-reviewers,
I think I know what caused postcommit failures after installing spirv-tools on CI:

So, in llvm-spirv, we detect spirv-tools using two methods: using pkg-config (main) or cmake (fallback)(https://github.com/intel/llvm/blob/sycl/llvm-spirv/CMakeLists.txt#L123)

  1. Using pkg-config:
    Our CI Linux container does not have pkg-config installed, unlike internally or in Podman's Ubuntu24 image.
    So, if I just install pkg-config in the docker container, we are able to correctly find spirv-tools and llvm-spirv LIT tests passes correctly.

  2. Using cmake:
    Since we did not have pkg-config installed, llvm-spirv was using cmake to find spirv-tools. However, I think there's a bug in spiv-tools's official Ubuntu 24 installation package that we obtained via:

. /etc/os-release
curl -L "https://packages.lunarg.com/lunarg-signing-key-pub.asc" | apt-key add -
echo "deb https://packages.lunarg.com/vulkan $VERSION_CODENAME main" | tee -a /etc/apt/sources.list
apt update && apt install -yqq spirv-tools

The installation package does not have the correct CMake target import file. Here's the import files apt install spirv-tools installed:

$:/usr/lib/x86_64-linux-gnu/cmake/SPIRV-Tools-tools# ls
SPIRV-Tools-toolsConfig.cmake   SPIRV-Tools-toolsTargets.cmake  SPIRV-Tools-toolsTargets-none.cmake

Notice the SPIRV-Tools-toolsTargets-none.cmake file - the none here refers to the build config. So, if we do get_target_property(SPIRV_AS_PATH spirv-as IMPORTED_LOCATION_RELEASE) or get_target_property(SPIRV_AS_PATH spirv-as IMPORTED_LOCATION_DEBUG) (that we use to get spirv-as path: https://github.com/intel/llvm/blob/sycl/llvm-spirv/test/CMakeLists.txt#L56), it will not load this file because none is an invalid build config for LLVM project. If I rename this file, SPIRV-Tools-toolsTargets-none.cmake --> SPIRV-Tools-toolsTargets-debug.cmake and manually replace NONE to DEBUG in build config, cmake correctly loads the file and all llvm-spriv LIT tests correct passes. Btw, if we build spirv-tools locally, it produces the SPIRV-Tools-toolsTargets-debug.cmake or SPIRV-Tools-toolsTargets-release.cmake file, depending on build config.
So, that's why I think there's a bug in spiv-tools official installation package.

Solution:
For the time being, I think we can just install pkg-config in the CI container - we also use pkg-config internally, plus, it will take some time to confirm and get the cmake bug fixed, which, at best, will happen in the next spirv-tools release.

PS:- I've installed spirv-tools on Windows CI machines and it is working fine: https://github.com/intel/llvm/actions/runs/12934269927/job/36192622174#step:14:1

@maarquitos14
Copy link
Contributor

@uditagarwal97 Thanks for the thorough investigation! I'm good with the suggested solution, let me know if you need any help.

sarnex pushed a commit that referenced this pull request Jan 27, 2025
`pkg-config` is required for `llvm-spirv` to detect `spirv-tools`
installation.
See #16743 (comment) for
more info.
uditagarwal97 added a commit that referenced this pull request Jan 27, 2025
ayylol added a commit to ayylol/intel-llvm that referenced this pull request Jan 28, 2025
commit 7dec039
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Jan 27 10:21:05 2025 -0800

    remove IntelGPU folder, and add target-spir to lit.local.cfg

commit dccac19
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Jan 27 10:09:58 2025 -0800

    Use target features instead of backend features

commit be87b96
Merge: d83a37d e923fbb
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Jan 27 10:06:53 2025 -0800

    Merge branch 'sycl' into inlineasm-litlocal

commit e923fbb
Author: Nick Sarnie <[email protected]>
Date:   Tue Jan 28 03:01:42 2025 +0900

    [CI] Update dev-igc even if IGC CI failed (intel#16790)

    IGC public CI has been failing for months and we got confirmation even
    if the public CI fails the change has already been internally validated.
    We can confirm this change works as below:
    [Old
    link](https://api.github.com/repos/intel/intel-graphics-compiler/actions/workflows/build-IGC.yml/runs?status=success)
    [New
    link](https://api.github.com/repos/intel/intel-graphics-compiler/actions/workflows/build-IGC.yml/runs)

    Signed-off-by: Sarnie, Nick <[email protected]>

commit 4eb095e
Author: Chris Perkins <[email protected]>
Date:   Mon Jan 27 09:34:35 2025 -0800

    [SYCL] remove experimental online_compiler extension (intel#16776)

    Minus support for "CM", all the functionality of the old experimental
    online_compiler extension is being provided by the new (also
    experimental) kernel_compiler. Only better.

    Besides always being merely experimental, the online_compiler has been
    marked as deprecated for over a year, it has become burdensome to
    continue to support, and it sometimes confuses users. The decision has
    been made to remove it, without waiting for an ABI breaking window.

commit 566f514
Author: Udit Kumar Agarwal <[email protected]>
Date:   Mon Jan 27 09:27:27 2025 -0800

    [CI] Install `pkg-config` in docker container (intel#16797)

    `pkg-config` is required for `llvm-spirv` to detect `spirv-tools`
    installation.
    See intel#16743 (comment) for
    more info.

commit 367f355
Author: Mészáros Gergely <[email protected]>
Date:   Mon Jan 27 17:28:39 2025 +0100

    [SYCL][E2E][NFC] Fix NameError if directive fails to parse (intel#16767)

    Currently if a test contains a malformed directive, the following
    exception is raised along with the original parsing error:
    ```plaintext
    Exception during script execution:
    (original error)

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "llvm/llvm/utils/lit/lit/worker.py", line 76, in _execute_test_handle_errors
        result = test.config.test_format.execute(test, lit_config)
      File "llvm/sycl/test-e2e/format.py", line 232, in execute
        script = self.parseTestScript(test)
      File "llvm/sycl/test-e2e/format.py", line 105, in parseTestScript
        return lit.Test.Result(Test.UNRESOLVED, str(e))
    NameError: name 'Test' is not defined
    ```
    The test ends up as UNRESOLVED either way, but fixing it is easy and
    improves the error message greatly.

commit d83a37d
Author: Garcia Orozco, David <[email protected]>
Date:   Wed Jan 22 13:47:59 2025 -0800

    Add `InlineAsm/IntelGPU` test directory

commit 745d9b5
Merge: e91210e 34ef866
Author: Garcia Orozco, David <[email protected]>
Date:   Wed Jan 22 13:33:25 2025 -0800

    Merge branch 'sycl' into inlineasm-litlocal

commit e91210e
Merge: 8d5f517 38e6e1b
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Jan 20 11:59:42 2025 -0800

    Merge branch 'sycl' into inlineasm-litlocal

commit 8d5f517
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Dec 9 11:51:56 2024 -0800

    Add REQUIRES: gpu,linux to lit.local.cfg

commit a518866
Author: Garcia Orozco, David <[email protected]>
Date:   Mon Dec 9 11:44:21 2024 -0800

    Use lit.local.cfg to mark cuda and hip as unsupported
sarnex pushed a commit that referenced this pull request Jan 28, 2025
…#16801)

Reverts #16743. After installing `pkg-config` in CI,
`llvm-spirv` can correctly find `spriv-tools` so this workaround is no
longer needed.
Example run:
https://github.com/intel/llvm/actions/runs/12996166493/job/36244369469?pr=16801#step:14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants