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

Track all customizations that are made in llvm-spirv in intel/llvm #7592

Open
7 of 9 tasks
MrSidims opened this issue Nov 30, 2022 · 13 comments
Open
7 of 9 tasks

Track all customizations that are made in llvm-spirv in intel/llvm #7592

MrSidims opened this issue Nov 30, 2022 · 13 comments
Assignees
Labels
confirmed enhancement New feature or request SPIR-V Issues related to SPIRV-LLVM-Translator

Comments

@MrSidims
Copy link
Contributor

MrSidims commented Nov 30, 2022

The goal is to keep intel/llvm's llvm-spirv on par with https://github.com/KhronosGroup/SPIRV-LLVM-Translator as much as possible. I'm not expecting it to be always on par, since some patches in https://github.com/KhronosGroup/SPIRV-LLVM-Translator requires appropriate update on devices' drivers in CI here, so the list should be updated.

Following patches are missing in this repository in comparison to https://github.com/KhronosGroup/SPIRV-LLVM-Translator :

This repository adds extra code:

This repository removes some code:

  • 1. This PR adds W/A that removes FunctionParameterAttributeNoReadWrite, need to return it.
  • 2. This PR Removes binding between FPFastMathModeINTEL capability and SPV_INTEL_fp_fast_math_mode extension.
@MrSidims MrSidims added the enhancement New feature or request label Nov 30, 2022
@MrSidims MrSidims self-assigned this Nov 30, 2022
@MrSidims
Copy link
Contributor Author

@vmaksimo please take a look and add notes if I'm missing something.

@MrSidims MrSidims changed the title Track all customizations that makes llvm-spirv in intel/llvm Track all customizations that are made llvm-spirv in intel/llvm Nov 30, 2022
@MrSidims MrSidims changed the title Track all customizations that are made llvm-spirv in intel/llvm Track all customizations that are made in llvm-spirv in intel/llvm Nov 30, 2022
@bader
Copy link
Contributor

bader commented Dec 2, 2022

The goal is to keep intel/llvm's llvm-spirv on pair with https://github.com/KhronosGroup/SPIRV-LLVM-Translator as much as possible. I'm not expecting it to be always on pair, since some patches in https://github.com/KhronosGroup/SPIRV-LLVM-Translator requires appropriate update on devices' drivers in CI here, so the list should be updated.

In my opinion we should remove llvm-spirv sources from intel/llvm repository. It difficult to track the difference between sources in Khronos and intel repositories. Custom layout makes it hard to sync and maintain. I suggest we consider:

  1. Add an intel branch in Khronos repository which should aim to be "always on par" with the main development branch but can be used to keep patches for Intel device. Non-SYCL compilers targeting Intel devices should benefit from this branch as well. If Khronos is against vendor branches in their repository, we can consider using a fork under intel organization.
  2. Keep set of patches on top of Khronos main branch in intel/llvm repository instead of the copy of sources. Keeping track of changes should be a no-brainer.

@vmaksimo
Copy link
Contributor

please take a look and add notes if I'm missing something.

In this repo we also have DecorationFuncParamDescINTEL and DecorationFuncParamKindINTEL, which are not present in Khronos. Other things mentioned are the same as in #3686 issue, I'll close it as a duplicate.

@asudarsa
Copy link
Contributor

asudarsa commented Apr 1, 2023

The goal is to keep intel/llvm's llvm-spirv on par with https://github.com/KhronosGroup/SPIRV-LLVM-Translator as much as possible. I'm not expecting it to be always on par, since some patches in https://github.com/KhronosGroup/SPIRV-LLVM-Translator requires appropriate update on devices' drivers in CI here, so the list should be updated.

Following patches are missing in this repository in comparison to https://github.com/KhronosGroup/SPIRV-LLVM-Translator :

This repository adds extra code:

This repository removes some code:

  • 1. This PR adds W/A that removes FunctionParameterAttributeNoReadWrite, need to return it.

Hi @MrSidims

I see PR in #1 (Add SPIR-V 1.4 checks) has been merged. Can we proceed with the remaining pointers here?

Thanks

@bader
Copy link
Contributor

bader commented Sep 14, 2023

@MrSidims, @asudarsa, #11186 - it seems like we missed a commit made 3 years ago. Can you check if anything else is missing, please?

@AlexeySachkov
Copy link
Contributor

Linking pulldown PR #11920 in here and in particular this comment and this commit - it should be reverted once KhronosGroup/SPIRV-LLVM-Translator#2224 is resolved. @jsji, @MrSidims: FYI

@jsji
Copy link
Contributor

jsji commented Mar 1, 2024

KhronosGroup/SPIRV-LLVM-Translator@64fe75e388dc3b7 is reverted in #12868 due to failures .
Failed Tests (3):
SYCL :: NonUniformGroups/ballot_group_algorithms.cpp
SYCL :: NonUniformGroups/opportunistic_group_algorithms.cpp
SYCL :: NonUniformGroups/tangle_group_algorithms.cpp

"[7:34 PM] Sidorov, Dmitry
I was checking if we indeed pass runtime value and it's not some sort of missing constant propagation. Unfortunately id passed to OpGroupNonUniformBroadcast is always runtime value for SYCL headers. Hence we need to revert the patch. "

@asudarsa
Copy link
Contributor

One more:
bbf3800

@jsji
Copy link
Contributor

jsji commented Mar 13, 2024

One more: bbf3800

Yes, this is the one mentioned above #7592 (comment)

@asudarsa
Copy link
Contributor

Ah..Thanks. I missed that. Will be addressing these soon.

dm-vodopyanov pushed a commit that referenced this issue Jul 10, 2024
This reverts commit ead8404.

The commit was W/A which should be removed to align with Khronos
translator code base.
It resolves p.2 of #7592 "adds extra code" section.
@maarquitos14
Copy link
Contributor

#15298 reverts the W/A that removes FunctionParameterAttributeNoReadWrite. Marked as solved in the description the corresponding item.

@MrSidims
Copy link
Contributor Author

Another customization was removed in #15882

@uditagarwal97
Copy link
Contributor

Added disable-spirv-tools LIT parameter to llvm-spirv tests to explicitly disable using spirv-tools in intel/llvm CI (#16743). These changes will be reverted once the test failures are fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed enhancement New feature or request SPIR-V Issues related to SPIRV-LLVM-Translator
Projects
None yet
Development

No branches or pull requests

9 participants