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

Support for SPV_INTEL_cache_controls #2140

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

aratajew
Copy link
Contributor

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Overall LGTM
Would it be possible to decorate user function calls that affect memory? Is it possible to decorate LifetimeStart/End instructions (this case seems to be unhandled).

@@ -87,7 +87,9 @@ enum InternalDecoration {
IDecHostAccessINTEL = 6147,
IDecInitModeINTEL = 6148,
IDecImplementInCSRINTEL = 6149,
IDecArgumentAttributeINTEL = 6409
IDecArgumentAttributeINTEL = 6409,
Copy link
Contributor

Choose a reason for hiding this comment

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

As the spec is public, please move these definitions to https://github.com/KhronosGroup/SPIRV-Headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created PR for SPIRV-Headers: KhronosGroup/SPIRV-Headers#376

; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls %t.bc -o %t.spv
; RUN: llvm-spirv -r %t.spv --spirv-target-env=SPV-IR -o - | llvm-dis -o - | FileCheck %s --check-prefix=CHECK-LLVM

; CHECK-SPIRV-DAG: Load {{[0-9]+}} {{[0-9]+}} [[LoadPtr:[0-9]+]]
Copy link
Contributor

Choose a reason for hiding this comment

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

We kinda decorate ***AccessChain*** instructions, should we add them in the check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's necessary. We know that Load's 2nd parameter is 'OpTypePointer' type and we want to make sure that it's properly decorated. It doesn't matter from what instruction the pointer comes from.

@aratajew
Copy link
Contributor Author

aratajew commented Sep 4, 2023

Would it be possible to decorate user function calls that affect memory? Is it possible to decorate LifetimeStart/End instructions (this case seems to be unhandled).

I'm not sure if I understand the question. The decoration is applied to OpTypePointer, not to instructions and it has only effect if the decorated pointer is directly used by load/store instruction.

@MrSidims
Copy link
Contributor

MrSidims commented Sep 4, 2023

Would it be possible to decorate user function calls that affect memory? Is it possible to decorate LifetimeStart/End instructions (this case seems to be unhandled).

I'm not sure if I understand the question. The decoration is applied to OpTypePointer, not to instructions and it has only effect if the decorated pointer is directly used by load/store instruction.

No, it's being applied not to OpTypePointer but to an object of this type, see the test:

; CHECK-SPIRV-DAG: Load {{[0-9]+}} {{[0-9]+}} [[**LoadPtr**:[0-9]+]]
; CHECK-SPIRV-DAG: Decorate [[LoadPtr]] CacheControlLoadINTEL 0 1

and OpLoad definition is the following:

OpLoad
Pointer is the pointer to load through. **Its type must** be an OpTypePointer whose Type operand is the
same as Result Type.
4 + variable 61 <id>| Result Type | Result <id> <id> | **Pointer** |Optional Memory Operands

So Pointer is not a type, but a result id of an instruction, that can create an object of OpTypePointer, like alloca / OpVariable or AccessChain instruction or LifetimeStart instruction or some other instruction.

UPD probably it doesn't matter, as actually LifetimeStart doesn't return a pointer, but just specifies when it's defined. Yet a question about OpFuctionCall returning a pointer remains

@aratajew
Copy link
Contributor Author

aratajew commented Sep 4, 2023

Would it be possible to decorate user function calls that affect memory? Is it possible to decorate LifetimeStart/End instructions (this case seems to be unhandled).

I'm not sure if I understand the question. The decoration is applied to OpTypePointer, not to instructions and it has only effect if the decorated pointer is directly used by load/store instruction.

No, it's being applied not to OpTypePointer but to an object of this type, see the test:

; CHECK-SPIRV-DAG: Load {{[0-9]+}} {{[0-9]+}} [[**LoadPtr**:[0-9]+]]
; CHECK-SPIRV-DAG: Decorate [[LoadPtr]] CacheControlLoadINTEL 0 1

and OpLoad definition is the following:

OpLoad
Pointer is the pointer to load through. **Its type must** be an OpTypePointer whose Type operand is the
same as Result Type.
4 + variable 61 <id>| Result Type | Result <id> <id> | **Pointer** |Optional Memory Operands

So Pointer is not a type, but a result id of an instruction, that can create an object of OpTypePointer, like alloca / OpVariable or AccessChain instruction or LifetimeStart instruction or some other instruction.

UPD probably it doesn't matter, as actually LifetimeStart doesn't return a pointer, but just specifies when it's defined. Yet a question about OpFuctionCall returning a pointer remains

Right, that's what I meant, just not said it precisely enough. Every object of type OpTypePointer can by decorated, so OpFunctionCall returning a pointer can also be decorated.

@MrSidims MrSidims merged commit 387841b into KhronosGroup:main Sep 4, 2023
8 checks passed
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 4, 2023
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 4, 2023
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 5, 2023
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 5, 2023
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 5, 2023
aratajew added a commit to aratajew/SPIRV-LLVM-Translator that referenced this pull request Sep 5, 2023
AlexeySachkov pushed a commit that referenced this pull request Sep 12, 2023
AlexeySachkov pushed a commit that referenced this pull request Sep 12, 2023
AlexeySachkov pushed a commit that referenced this pull request Sep 12, 2023
AlexeySachkov pushed a commit that referenced this pull request Sep 12, 2023
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.

2 participants