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

llvm-spirv: Set Opaque Pointers support also when compiling LLVM to SPIR-V #2069

Open
wants to merge 1 commit into
base: llvm_release_160
Choose a base branch
from

Conversation

karolherbst
Copy link
Contributor

LLVM implicitly upgrades modules to opaque pointers, so we have to do it also when translating LLVM to SPIR-V.

This should also get backported to branches containing 0fe726f (and backports).

@karolherbst
Copy link
Contributor Author

Seems like a lot of tests are failing, because they expect -emit-opaque-pointers to be set. Maybe it's better to only do this for the llvm-16 based branch for now and hope everything is going to be fixed with llvm-17.

But maybe fixing the translator is the better approach here, though for llvm-16 I think it's better to fallback to keep opaque pointers disabled.

…PIR-V

LLVM implicitly upgrades modules to opaque pointers, so we have to do it
also when translating LLVM to SPIR-V.
@karolherbst karolherbst changed the base branch from main to llvm_release_160 July 1, 2023 12:57
@karolherbst
Copy link
Contributor Author

maybe we even want to disable it by default as this might break users? Not quite sure. Any thoughts on this?

@MrSidims
Copy link
Contributor

MrSidims commented Jul 3, 2023

AFAIK opaque pointers are not default for LLVM 16, so I'd vote not to enable them here. @jcranmer-intel WDYT?

@karolherbst
Copy link
Contributor Author

AFAIK opaque pointers are not default for LLVM 16, so I'd vote not to enable them here. @jcranmer-intel WDYT?

they are on by default though, so that's why I need this change so the LLVMContext knows the translator wants them disabled.

LLVM tooling implicitly upgrades to opaque pointers in its tools as well (e.g. link or opt) unless you state you want them disabled, but that's a different problem.

It's already set for the "SPIRV to LLVM" path, just not in the "LLVM to SPIRV" one

@jcranmer-intel
Copy link
Contributor

I've created #2074 for flipping the default of -emit-opaque-pointers on trunk.

The proper way to set the flag for LLVM-to-SPIR-V translation is to use the -opaque-pointers=0 flag for versions that don't autodetect LLVM IR on output (which is 16 and newer). I don't see a need to add a redundant -emit-opaque-pointers=0 for the forward translation, especially given that the name is inherently wrong.

@karolherbst
Copy link
Contributor Author

I've created #2074 for flipping the default of -emit-opaque-pointers on trunk.

The proper way to set the flag for LLVM-to-SPIR-V translation is to use the -opaque-pointers=0 flag for versions that don't autodetect LLVM IR on output (which is 16 and newer). I don't see a need to add a redundant -emit-opaque-pointers=0 for the forward translation, especially given that the name is inherently wrong.

but that doesn't work? llvm-spirv consumes the LLVM IR (even with opaque pointers disabled) and upgrades it to opaque pointers breaking various things translating to SPIR-V without this change.

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.

3 participants