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

[mlir][spirv] Support function argument decorations #76106

Closed
kuhar opened this issue Dec 20, 2023 · 10 comments · Fixed by #76353
Closed

[mlir][spirv] Support function argument decorations #76106

kuhar opened this issue Dec 20, 2023 · 10 comments · Fixed by #76353
Assignees
Labels
good first issue https://github.com/llvm/llvm-project/contribute help wanted Indicates that a maintainer wants help. Not [good first issue]. mlir:spirv

Comments

@kuhar
Copy link
Member

kuhar commented Dec 20, 2023

We need to support function argument decorations in spirv.func to allow for some pointer types. For example, the spec requires the following:

If an OpFunctionParameter is a pointer (or array of pointers) in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of Aliased or Restrict.

If an OpFunctionParameter is a pointer (or array of pointers) and its pointee type is a pointer in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of AliasedPointer or RestrictPointer.

Note that this is already supported with the spirv.Variable op using the following syntax:

    %3 = spirv.Variable {aliased_pointer} :
      !spirv.ptr<!spirv.ptr<f32, PhysicalStorageBuffer>, Function>
@kuhar kuhar added help wanted Indicates that a maintainer wants help. Not [good first issue]. good first issue https://github.com/llvm/llvm-project/contribute mlir:spirv labels Dec 20, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 20, 2023

@llvm/issue-subscribers-mlir-spirv

Author: Jakub Kuderski (kuhar)

We need to support function argument decorations in `spirv.func` to allow for some pointer types. For example, the spec requires the following:

> If an OpFunctionParameter is a pointer (or array of pointers) in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of Aliased or Restrict.

> If an OpFunctionParameter is a pointer (or array of pointers) and its pointee type is a pointer in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of AliasedPointer or RestrictPointer.

Note that this is already supported with the spirv.Variable op using the following syntax:

    %3 = spirv.Variable {aliased_pointer} :
      !spirv.ptr&lt;!spirv.ptr&lt;f32, PhysicalStorageBuffer&gt;, Function&gt;

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 20, 2023

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 20, 2023

@llvm/issue-subscribers-good-first-issue

Author: Jakub Kuderski (kuhar)

We need to support function argument decorations in `spirv.func` to allow for some pointer types. For example, the spec requires the following:

> If an OpFunctionParameter is a pointer (or array of pointers) in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of Aliased or Restrict.

> If an OpFunctionParameter is a pointer (or array of pointers) and its pointee type is a pointer in PhysicalStorageBuffer storage class, then the function parameter must be decorated with exactly one of AliasedPointer or RestrictPointer.

Note that this is already supported with the spirv.Variable op using the following syntax:

    %3 = spirv.Variable {aliased_pointer} :
      !spirv.ptr&lt;!spirv.ptr&lt;f32, PhysicalStorageBuffer&gt;, Function&gt;

@sott0n
Copy link
Contributor

sott0n commented Dec 21, 2023

Hi, I'm new to the SPIRV dialect, so I'd like to work on this issue.

@kuhar
Copy link
Member Author

kuhar commented Dec 21, 2023

Hi, I'm new to the SPIRV dialect, so I'd like to work on this issue.

Sure @sott0n, feel free to ping me if you need any pointers.

@kuhar
Copy link
Member Author

kuhar commented Dec 21, 2023

A good starting point would be llvm-project/mlir/test/Dialect/SPIRV/IR/target-and-abi.mlir -- we already support some argument attributes like:

func.func @interface_var(
    %arg0 : f32 {spirv.interface_var_abi = #spirv.interface_var_abi<(0, 1), Uniform>}
) { return }

so what we need to do is to enable the same thing for decoration attributes and make sure we serialize them as OpDecorate over OpFunctionParameter. The following IR be accepted by mlir-opt

spirv.module PhysicalStorageBuffer64 GLSL450 requires #spirv.vce<v1.3,
    [Shader, PhysicalStorageBufferAddresses], [SPV_KHR_physical_storage_buffer]> {

  spirv.func @access(%arg0 : !spirv.ptr<!spirv.struct<(f32)>, PhysicalStorageBuffer> { spirv.decoration = #spirv.decoration<Aliased>}) -> f32 "None" {
    %idx = spirv.Constant 0 : i32
    %result = spirv.AccessChain %arg0[%idx] : !spirv.ptr<!spirv.struct<(f32)>, PhysicalStorageBuffer>, i32
    %ptr = spirv.Load "PhysicalStorageBuffer" %result : f32
    spirv.ReturnValue %ptr : f32
  }
}

and pass validation with:

bin/mlir-translate --no-implicit-module --serialize-spirv test.mlir  -o test.spv
spirv-val test.spv

@sott0n
Copy link
Contributor

sott0n commented Dec 25, 2023

I'm working on #76353

I have completed support for function argument decoration (I'm going to refactor it) and am now working on implementing serialization to pass SPIR-V's validation via spirv-val.

@sott0n
Copy link
Contributor

sott0n commented Dec 28, 2023

I've finished implementing the serializer and deserializer. However, spirv-val is causing an error with the OpLoad. For the physical buffer pointer, the SPIR-V dialect needs to add the Aligned with an OpLoad/OpStore (i'm going to try it after this PR).

SPIRV validation tool's error:

spirv-val test.spv
error: line 19: Memory accesses with PhysicalStorageBuffer must use Aligned.
  %12 = OpLoad %float %11

This PR supported the decoration for the function's parameters (OpDecorate %6 Aliased), but not yet implemented the Aligned in the OpLoad when the operand is the physical buffer pointer.

spirv-dis test.spv                                                                                            
; SPIR-V
; Version: 1.3
; Generator: Khronos; 22
; Bound: 13
; Schema: 0
               OpCapability Shader
               OpCapability PhysicalStorageBufferAddresses
               OpExtension "SPV_KHR_physical_storage_buffer"
               OpMemoryModel PhysicalStorageBuffer64 GLSL450
               OpName %access "access"
               OpDecorate %_struct_4 Block
               OpDecorate %6 Aliased
      %float = OpTypeFloat 32
  %_struct_4 = OpTypeStruct %float
%_ptr_PhysicalStorageBuffer__struct_4 = OpTypePointer PhysicalStorageBuffer %_struct_4
          %1 = OpTypeFunction %float %_ptr_PhysicalStorageBuffer__struct_4
       %uint = OpTypeInt 32 0
     %uint_0 = OpConstant %uint 0
%_ptr_PhysicalStorageBuffer_float = OpTypePointer PhysicalStorageBuffer %float
     %access = OpFunction %float None %1
          %6 = OpFunctionParameter %_ptr_PhysicalStorageBuffer__struct_4
          %7 = OpLabel
         %11 = OpAccessChain %_ptr_PhysicalStorageBuffer_float %6 %uint_0
         %12 = OpLoad %float %11
               OpReturnValue %12

@kuhar
Copy link
Member Author

kuhar commented Dec 28, 2023

This PR supported the decoration for the function's parameters (OpDecorate %6 Aliased), but not yet implemented the Aligned in the OpLoad when the operand is the physical buffer pointer.

Good point @sott0n, I think it's just a bug in my example though, isn't it? The input mlir should have been:

%ptr = spirv.Load "PhysicalStorageBuffer" %result ["Aligned", 4] : f32

@sott0n
Copy link
Contributor

sott0n commented Dec 29, 2023

Yes, I think it's correct. I updated your example and this error is no longer there.

antiagainst added a commit that referenced this issue Jan 6, 2024
…76353)

Closes #76106

---------

Co-authored-by: Lei Zhang <antiagainst@gmail.com>
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this issue Jan 28, 2024
…lvm#76353)

Closes llvm#76106

---------

Co-authored-by: Lei Zhang <antiagainst@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue https://github.com/llvm/llvm-project/contribute help wanted Indicates that a maintainer wants help. Not [good first issue]. mlir:spirv
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants