[mlir][spirv] Update assembly format for Image operand types #130758

IgWod-IMG · 2025-03-11T12:01:41Z

In the example below it is not clear that (f32) relates to %arg2 and not to vector<2xf32>:

%0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"](%arg2) :
  !spirv.sampled_image<...>, vector<2xf32>(f32) -> vector<4xf32>

This change applies new format to image operations and image operands that does not use parenthesis and is less ambiguous:

%0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"], %arg2 :
  !spirv.sampled_image<...>, vector<2xf32>, f32 -> vector<4xf32>

llvmbot · 2025-03-11T12:02:16Z

@llvm/pr-subscribers-mlir

Author: Igor Wodiany (IgWod-IMG)

Changes

In the example below it is not clear that (f32) relates to %arg2 and not to vector<2xf32>. This change makes it more clear by separating (f32) from vector<...>.

%0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"](%arg2) :
  !spirv.sampled_image&lt;...&gt;, vector&lt;2xf32&gt;(f32) -&gt; vector&lt;4xf32&gt;

Full diff: https://github.com/llvm/llvm-project/pull/130758.diff

1 Files Affected:

(modified) mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td (+2-2)

diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
index a4fe29536e60a..2c5714800493f 100644
--- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
+++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
@@ -115,7 +115,7 @@ def SPIRV_ImageDrefGatherOp : SPIRV_Op<"ImageDrefGather",
 
   let assemblyFormat = [{
     $sampled_image `,` $coordinate `,` $dref custom<ImageOperands>($image_operands) ( `(` $operand_arguments^ `)` )? attr-dict 
-    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ( `(` type($operand_arguments)^ `)` )?
+    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ` ` ( `(` type($operand_arguments)^ `)` )?
     `->` type($result) 
   }];
 
@@ -227,7 +227,7 @@ def SPIRV_ImageWriteOp : SPIRV_Op<"ImageWrite",
 
   let assemblyFormat = [{
     $image `,` $coordinate `,` $texel custom<ImageOperands>($image_operands) ( `(` $operand_arguments^ `)`)? attr-dict
-    `:` type($image) `,` type($coordinate) `,` type($texel) ( `(` type($operand_arguments)^ `)`)?
+    `:` type($image) `,` type($coordinate) `,` type($texel) ` ` ( `(` type($operand_arguments)^ `)`)?
   }];
 }

llvmbot · 2025-03-11T12:02:17Z

@llvm/pr-subscribers-mlir-spirv

Author: Igor Wodiany (IgWod-IMG)

Changes

In the example below it is not clear that (f32) relates to %arg2 and not to vector<2xf32>. This change makes it more clear by separating (f32) from vector<...>.

%0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"](%arg2) :
  !spirv.sampled_image&lt;...&gt;, vector&lt;2xf32&gt;(f32) -&gt; vector&lt;4xf32&gt;

Full diff: https://github.com/llvm/llvm-project/pull/130758.diff

1 Files Affected:

(modified) mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td (+2-2)

diff --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
index a4fe29536e60a..2c5714800493f 100644
--- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
+++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td
@@ -115,7 +115,7 @@ def SPIRV_ImageDrefGatherOp : SPIRV_Op<"ImageDrefGather",
 
   let assemblyFormat = [{
     $sampled_image `,` $coordinate `,` $dref custom<ImageOperands>($image_operands) ( `(` $operand_arguments^ `)` )? attr-dict 
-    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ( `(` type($operand_arguments)^ `)` )?
+    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ` ` ( `(` type($operand_arguments)^ `)` )?
     `->` type($result) 
   }];
 
@@ -227,7 +227,7 @@ def SPIRV_ImageWriteOp : SPIRV_Op<"ImageWrite",
 
   let assemblyFormat = [{
     $image `,` $coordinate `,` $texel custom<ImageOperands>($image_operands) ( `(` $operand_arguments^ `)`)? attr-dict
-    `:` type($image) `,` type($coordinate) `,` type($texel) ( `(` type($operand_arguments)^ `)`)?
+    `:` type($image) `,` type($coordinate) `,` type($texel) ` ` ( `(` type($operand_arguments)^ `)`)?
   }];
 }

IgWod-IMG · 2025-03-11T12:04:51Z

I kept the code as it was in #129558 for now on.

kuhar · 2025-03-12T14:55:16Z

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVImageOps.td

@@ -115,7 +115,7 @@ def SPIRV_ImageDrefGatherOp : SPIRV_Op<"ImageDrefGather",

  let assemblyFormat = [{
    $sampled_image `,` $coordinate `,` $dref custom<ImageOperands>($image_operands) ( `(` $operand_arguments^ `)` )? attr-dict 
-    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ( `(` type($operand_arguments)^ `)` )?
+    `:` type($sampled_image) `,` type($coordinate) `,` type($dref) ` ` ( `(` type($operand_arguments)^ `)` )?


Instead of adding spaces to the format, could we start converging towards a common format with T0, T1, T2 -> T3? This won't make the grammar ambiguous, AFAICT.

IE having parens in the input portion of the op type seems to be the root cause of our issues here, and I'd prefer to drop them.

Before I update the patch. Is this the format we're after (using an example):

%0 = spirv.Op %1, %2 ["IOp1 | IOp2"] %3, %4, %5 : !spirv.sampled_image<...>, vector<2xf32>, f32, f32, f32 -> vector<4xf32>

Also, an example form an actual shader:

%6 = spirv.ImageSampleExplicitLod %3, %4 ["Lod"] %5 : !spirv.sampled_image<!spirv.image<f32, Dim2D, NoDepth, NonArrayed, SingleSampled, NeedSampler, Unknown>>, vector<2xf32>, f32 -> vector<4xf32>

It drops parens after the image operands and keeps image operands as they were. Then it lists all the types with commas.

I think it works quite well and it's clear to read.

Also, do we want to merge #129558 first and I'll fix everything here or merge this PR first, and I'll update the other PR?

Yes, just like in your example, We can also add a comma after the closing bracket. I'd prefer to update the existing op syntax first and then land the other PR that adds new ops with the same syntax

I'm not convinced about the comma after the bracket. If we look at the example with comma:

%0 = spirv.Op %1, %2 ["IOp1 | IOp2"], %3, %4, %5 : !spirv.sampled_image<...>, vector<2xf32>, f32, f32, f32 -> vector<4xf32>

This can imply that operands are attached to %2. Unless we put a comma before the bracket as well:

%0 = spirv.Op %1, %2, ["IOp1 | IOp2"], %3, %4, %5 : !spirv.sampled_image<...>, vector<2xf32>, f32, f32, f32 -> vector<4xf32>

But personally, I prefer no commas before or after operands list. But I have no strong feelings about it, so I'll let you decide.

I'd prefer to update the existing op syntax first and then land the other PR that adds new ops with the same syntax

Sounds good, it's my preferred solution as well.

OK, I don't have a strong preference here, the only thing is that I'd like to keep doing is making the syntax more uniform across the spirv dialect

IgWod-IMG · 2025-03-14T10:56:42Z

I have updated the PR with the agreed format.

kuhar · 2025-03-14T13:47:10Z

mlir/test/Dialect/SPIRV/IR/image-ops.mlir

@@ -22,7 +22,7 @@ func.func @image_dref_gather_with_single_imageoperands(%arg0 : !spirv.sampled_im

 func.func @image_dref_gather_with_mismatch_imageoperands(%arg0 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, %arg1 : vector<4xf32>, %arg2 : f32) -> () {
  // expected-error @+1 {{the Image Operands should encode what operands follow, as per Image Operands}}
-  %0 = spirv.ImageDrefGather %arg0, %arg1, %arg2 (%arg2, %arg2) : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32 (f32, f32) -> vector<4xi32>
+  %0 = spirv.ImageDrefGather %arg0, %arg1, %arg2 %arg2, %arg2 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32, f32, f32 -> vector<4xi32>


Suggested change

%0 = spirv.ImageDrefGather %arg0, %arg1, %arg2 %arg2, %arg2 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32, f32, f32 -> vector<4xi32>

%0 = spirv.ImageDrefGather %arg0, %arg1, %arg2, %arg2, %arg2 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32, f32, f32 -> vector<4xi32>

Could we add a comma here to match the type?

So, this will not parse with the current assembly format. But also, it's a degenerate case that mises Image Operands before optional arguments, hence the assembly looks weird in the test. So, we either keep it as it is now, or we can re-evaluate the format. Adding comma like you suggested before would make this a valid assembly that parses. So maybe we should go for this format in the end:

%0 = spirv.Op %1, %2 ["IOp1 | IOp2"], %3, %4, %5 : !spirv.sampled_image<...>, vector<2xf32>, f32, f32, f32 -> vector<4xf32>

With the current example, I can see it's useful to have this comma. If you're happy to go with the comma as you initially suggested, I'll update the patch.

OK, let's not overthink this, it doesn't have to be perfect. I'm happy with a small improvement over the status quo.

kuhar

Approving since we agreed on the format

In the example below it is not clear that `(f32)` relates to `%arg2` and not to `vector<2xf32>`: ```mlir %0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"](%arg2) : !spirv.sampled_image<...>, vector<2xf32>(f32) -> vector<4xf32> ``` This change applies new format to image operations and image operands that does not use parenthesis and is less ambiguous: ```mlir %0 = spirv.ImageSampleImplicitLod %arg0, %arg1 ["Lod"] %arg2 : !spirv.sampled_image<...>, vector<2xf32>, f32 -> vector<4xf32> ```

IgWod-IMG · 2025-03-14T16:34:10Z

I have added the comma and updated the test. I'll merge it once it passes tests (unless there're any objections by then).

IgWod-IMG requested review from antiagainst and kuhar as code owners March 11, 2025 12:01

llvmbot added mlir:spirv mlir labels Mar 11, 2025

IgWod-IMG mentioned this pull request Mar 11, 2025

[mlir][spirv] Add definition for selected sample operations #129558

Merged

kuhar reviewed Mar 12, 2025

View reviewed changes

IgWod-IMG force-pushed the img_image-assembly branch from cb3ae1c to 939a9b6 Compare March 14, 2025 10:54

kuhar reviewed Mar 14, 2025

View reviewed changes

kuhar approved these changes Mar 14, 2025

View reviewed changes

IgWod-IMG force-pushed the img_image-assembly branch from 939a9b6 to e22575b Compare March 14, 2025 16:30

IgWod-IMG merged commit ef9c4f4 into llvm:main Mar 24, 2025
11 checks passed

IgWod-IMG deleted the img_image-assembly branch March 24, 2025 09:31

	%0 = spirv.ImageDrefGather %arg0, %arg1, %arg2 %arg2, %arg2 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32, f32, f32 -> vector<4xi32>
	%0 = spirv.ImageDrefGather %arg0, %arg1, %arg2, %arg2, %arg2 : !spirv.sampled_image<!spirv.image<i32, Dim2D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>>, vector<4xf32>, f32, f32, f32 -> vector<4xi32>

[mlir][spirv] Update assembly format for Image operand types #130758

[mlir][spirv] Update assembly format for Image operand types #130758

Uh oh!

Conversation

IgWod-IMG commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 11, 2025

Uh oh!

llvmbot commented Mar 11, 2025

Uh oh!

IgWod-IMG commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG commented Mar 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhar Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhar left a comment

Choose a reason for hiding this comment

Uh oh!

IgWod-IMG commented Mar 14, 2025

Uh oh!

Uh oh!

Uh oh!

IgWod-IMG commented Mar 11, 2025 •

edited

Loading

kuhar Mar 14, 2025 •

edited

Loading