Translate nonsemantic attribute and metadata of GlobalVariable #2944
+165
−37
Conversation
This file contains 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
Motivations is similar as f729c49. This PR addresses SYCL device global which may have attributes "sycl-device-image-scope", "sycl-host-access" and "sycl-unique-id". Failure to preserve "sycl-unique-id" after llvm-spirv translation triggers assert error at https://github.com/intel/llvm/blob/2824f61dd36790448a224cd596985bd01cbcd0f3/llvm/lib/SYCLLowerIR/DeviceGlobals.cpp#L85 Also preserve GlobalVariable metadata as an improvement, though there is no test to show this is really needed.
MrSidims
reviewed
Jan 7, 2025
lib/SPIRV/SPIRVReader.cpp
Outdated
Comment on lines
5146
to
5148
| auto *GO = cast<GlobalObject>(getTranslatedValue(Arg0)); | ||
| auto *F = dyn_cast_or_null<Function>(GO); | ||
| auto *GV = dyn_cast_or_null<GlobalVariable>(GO); |
There was a problem hiding this comment.
This makes me a little bit suspicious, why do we need dyn_cast**_or_null** here when GO is a result of a cast? May be it's leftover from a previous patch iteration, where some test was failing.
There was a problem hiding this comment.
done, yet it is a misuse.
lib/SPIRV/SPIRVWriter.cpp
Outdated
| auto *BM = BF->getModule(); | ||
| void LLVMToSPIRVBase::transAuxDataInst(SPIRVValue *BV, Value *V) { | ||
| auto *GO = cast<GlobalObject>(V); | ||
| auto *F = dyn_cast_or_null<Function>(GO); |
There was a problem hiding this comment.
Same question about _or_null
MrSidims
approved these changes
Jan 8, 2025
| PreserveCount = 2 | ||
| GlobalVariableMetadata = 2, | ||
| GlobalVariableAttribute = 3, | ||
| PreserveCount = 4 |
There was a problem hiding this comment.
If PreserveCount is (and has been previously) unused it should be probably deleted entirely. Otherwise, this line introduces a concern about binary incompatibility due to changing PreserveCount's value.
There was a problem hiding this comment.
deleted. It is unused.
VyacheslavLevytskyy
approved these changes
Jan 8, 2025
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.
Motivations is similar as f729c49. This PR addresses SYCL device global which may have attributes "sycl-device-image-scope", "sycl-host-access" and "sycl-unique-id". Failure to preserve "sycl-unique-id" after llvm-spirv translation triggers assert at
https://github.com/intel/llvm/blob/2824f61dd36790448a224cd596985bd01cbcd0f3/llvm/lib/SYCLLowerIR/DeviceGlobals.cpp#L85
Also preserve GlobalVariable metadata as an improvement, though there is no test to show this is really needed.