Validator pass for image instructions by atgoo · Pull Request #953 · KhronosGroup/SPIRV-Tools

atgoo · 2017-11-08T17:54:53Z

Includes validation rules for 22 OpImageXXX opcodes and ImageOperand.

Doesn't include OpTypeImage and OpImageSparseXXX.

Includes validation rules for OpImageXXX and ImageOperand. Doesn't include OpTypeImage and OpImageSparseXXX.

atgoo · 2017-11-08T17:58:18Z

Passes Vulkan CTS master.

dneto0

This is good, detailed work.

Many of the questionable bits match the spec issues you have filed, so I've not commented on them here.

I have filed issues for followup in some areas.

Otherwise, I'd like some small fixups, including adding checks in three places where the Cube dim is disallowed.

Thanks for your patience waiting for this review!

dneto0 · 2017-11-16T16:08:40Z

source/validate_image.cpp

+// this module. If SpvImageOperandsXXX list changes, this function will fail the
+// build.
+// For all other purposes this is a dummy function.
+bool CheckAllImageOperandsHandled() {


I'm glad you're checking this.

The approach is unorthodox. But I see it works.

dneto0 · 2017-11-16T16:13:24Z

source/validate_image.cpp

+}
+
+// Returns number of '1' bits in a word.
+uint32_t CountSetBits(uint32_t word) {


It's worth mentioning that compilers have builtins for this: GCC and Clang have __builtin_popcount, MSVC has __popcnt.
But probably not worth implementing the fast path. (Measure before optimizing)

dneto0 · 2017-11-16T16:58:25Z

source/validate_image.cpp

+  SpvAccessQualifier access_qualifier = SpvAccessQualifierMax;
+};
+
+// Provides information on image type.


Should also say that id can be either an OpTypeImage or OpTypeSampledImage
Explain the return value.

dneto0 · 2017-11-16T17:06:37Z

source/validate_image.cpp

+}
+
+// Returns actual dimensionality of |info.dim|.
+uint32_t GetPlaneSize(const ImageTypeInfo& info) {


I find this name confusing. Should it be GetPlaneCoordComponentCount or to match other names, GetPlaneCoordSize, GetPlaneDimension ?
And update the comment to "Returns the number of components in a coordinate used to access a texel in a single plane of an image with the given parameters." ???

I like that "Plane" is included because we're factoring out the array-ness of the image.

dneto0 · 2017-11-16T17:08:51Z

source/validate_image.cpp

+  return false;
+}
+
+bool IsProjLod(SpvOp opcode) {


Should rename to IsProj

These instructions are only different from others because they have "Proj".

dneto0 · 2017-11-16T19:43:22Z

source/validate_image.cpp

+      return _.diag(SPV_ERROR_INVALID_DATA)
+          << "Expected Image Operand ConstOffset to have " << plane_size
+          << " components, but given " << offset_size << ": "
+          << spvOpcodeString(opcode);


Need to reject if used with Cube dim.

dneto0 · 2017-11-16T19:55:43Z

source/validate_image.cpp

+      return _.diag(SPV_ERROR_INVALID_DATA)
+          << "Expected Image Operand ConstOffsets to be a const object: "
+          << spvOpcodeString(opcode);
+    }


Should reject if used with Cube dimension

dneto0 · 2017-11-16T19:56:16Z

source/validate_image.cpp

+          << "Expected Image Operand Offset to have " << plane_size
+          << " components, but given " << offset_size << ": "
+          << spvOpcodeString(opcode);
+    }


Should reject if used with Cube dimension

dneto0 · 2017-11-16T20:16:53Z

source/validate_image.cpp

+      }
+
+      ImageTypeInfo info;
+      if (!GetImageTypeInfo(_, _.GetOperandTypeId(inst, 2), &info)) {


Because GetImageTypeInfo will trace through an OpTypeSampledImage, it seems this will permit an OpSampledImage to have an image operand which itself has type OpTypeSampledImage.

I think the code must independently check that the image operand has type OpTypeImage

Done. I believe it is done so at other locations.

dneto0 · 2017-11-16T22:19:26Z

source/validate_image.cpp

+      }
+
+#if 0
+      // TODO(atgoo@github.com) The spec doesn't whitelist all Dims supported by


??
SPIR-V 1.2 seems to whitelist the dimensionality in the description of the Image argument:

Its Dim operand must be one of Rect or Buffer, or if its MS is 1, it can be 2D, or, if its Sampled Type is 0 or 2, it can be 2D or 3D.

So it's Rect, Buffer, 2D, 3D.

The same list is implied by the description of the result type.

But it doesn't whitelist all of them:
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/imageSize.xhtml

atgoo

I think I accidentally deleted one of the comments. Sorry about that.

atgoo · 2017-11-17T15:19:12Z

source/validate_image.cpp

+  }
+
+  if (mask & SpvImageOperandsLodMask) {
+    if (is_implcit_lod) {


Added a TODO. There is a spec issue about this, as the spec does allude that not only ExplicitLod opcodes are possible.

Lod
A following operand is the explicit level-of-detail to use. Only valid with explicit-lod instructions. For sampling operations, it must be a floating-point type scalar. For fetch operations, it must be an integer type scalar.

atgoo · 2017-11-17T15:34:03Z

source/validate_image.cpp

+      return _.diag(SPV_ERROR_INVALID_DATA)
+          << "Expected Image Operand ConstOffset to have " << plane_size
+          << " components, but given " << offset_size << ": "
+          << spvOpcodeString(opcode);


atgoo · 2017-11-17T15:40:51Z

source/validate_image.cpp

+          << "Expected Image Operand Offset to have " << plane_size
+          << " components, but given " << offset_size << ": "
+          << spvOpcodeString(opcode);
+    }


atgoo · 2017-11-17T15:46:43Z

source/validate_image.cpp

+      return _.diag(SPV_ERROR_INVALID_DATA)
+          << "Expected Image Operand ConstOffsets to be a const object: "
+          << spvOpcodeString(opcode);
+    }


atgoo · 2017-11-17T15:53:42Z

source/validate_image.cpp

+      }
+
+      ImageTypeInfo info;
+      if (!GetImageTypeInfo(_, _.GetOperandTypeId(inst, 2), &info)) {


Done. I believe it is done so at other locations.

atgoo · 2017-11-17T15:56:10Z

source/validate_image.cpp

+      }
+
+#if 0
+      // TODO(atgoo@github.com) The spec doesn't whitelist all Dims supported by


But it doesn't whitelist all of them:
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/imageSize.xhtml

atgoo · 2017-11-17T18:47:12Z

Feedback addressed.

dneto0

Thanks!

dneto0 · 2017-11-22T19:30:33Z

source/validate_image.cpp

 }

-bool IsProjLod(SpvOp opcode) {
+bool IdProj(SpvOp opcode) {


IsProj :-)

I can fix this before merge.

dneto0 · 2017-11-22T19:31:04Z

test/val/val_image_test.cpp

  const std::string body = R"(
 %img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001
 %sampler = OpLoad %type_sampler %uniform_sampler
-%simg = OpSampledImage %type_sampled_image_f32_2d_0001 %sampler %sampler


dneto0 · 2017-11-22T19:37:10Z

Rebased, squashed, typo fixed, and pushed into master as f407ae2

Roll third_party/glslang/ 40801e3..d203754 (6 commits) KhronosGroup/glslang@40801e3...d203754 $ git log 40801e3..d203754 --date=short --no-merges --format='%ad %ae %s' 2020-01-07 cepheus Fix KhronosGroup#1829: Add "--" command-line options for macro def/undef. 2020-01-08 laddoc Move symbol builtin check to grammar stage 2020-01-06 lryer Add missing extension defination 2019-11-26 laddoc Add support for ARB_gpu_shader_fp64 2020-01-03 lryer Fix glslang can't link multiple AST in a single stage 2019-12-25 laddoc Modify atomic_uint binding check Roll third_party/googletest/ 306f3754a..d854bd6ac (4 commits) google/googletest@306f375...d854bd6 $ git log 306f3754a..d854bd6ac --date=short --no-merges --format='%ad %ae %s' 2020-01-09 absl-team Googletest export 2020-01-09 absl-team Googletest export 2020-01-07 absl-team Googletest export 2020-01-07 absl-team Googletest export Roll third_party/re2/ 00af5b44d..85c014206 (2 commits) google/re2@00af5b4...85c0142 $ git log 00af5b44d..85c014206 --date=short --no-merges --format='%ad %ae %s' 2020-01-12 junyer Tidy up a test. 2020-01-07 junyer Prevent ShortVisit() from crashing fuzzers. Roll third_party/spirv-cross/ 961b9014a..172e39f03 (14 commits) KhronosGroup/SPIRV-Cross@961b901...172e39f $ git log 961b9014a..172e39f03 --date=short --no-merges --format='%ad %ae %s' 2020-01-09 post HLSL: Add a resource remapping API similar to MSL. 2020-01-09 post MSL: Deal with sign on wave min/max. 2020-01-09 post HLSL: Deal with casting for WaveActiveMin/Max. 2020-01-09 post GLSL: Deal with sign in subgroup Min/Max operations. 2020-01-08 post Run format_all.sh. 2020-01-08 post HLSL: Fix bug when reading and writing structs from SSBO. 2020-01-08 post HLSL: Implement stores for complex composites in ByteAddressBuffers. 2020-01-08 post HLSL: Support loading complex composites from ByteAddressBuffer. 2020-01-08 post Run format_all.sh. 2020-01-07 post MSL: Deal with padded fragment output + Component decoration. 2020-01-07 post MSL: Explicitly don't support component packing for tessellation. 2020-01-07 post MSL: Don't set OrigID when emitting component packed vectors. 2020-01-07 post MSL: Deal with packing vectors for vertex input/fragment output. 2020-01-07 post MSL: Add trivial tests for Component decoration. Roll third_party/spirv-tools/ c8bf143..18b3b94 (6 commits) KhronosGroup/SPIRV-Tools@c8bf143...18b3b94 $ git log c8bf143..18b3b94 --date=short --no-merges --format='%ad %ae %s' 2020-01-10 33791085+aqnuep Remove names and decorations of imported symbols (KhronosGroup#3081) 2020-01-08 dneto Fix GN build for OpenCL.DebugInfo.100 update (KhronosGroup#3134) 2020-01-08 bclayton Fix bad parameter names in error message (KhronosGroup#3129) 2020-01-07 alanbaker Revert PR KhronosGroup#3093 (KhronosGroup#3131) 2020-01-07 alanbaker Disallow forward references in arrays (KhronosGroup#3093) 2020-01-07 afdx spirv-fuzz: Add fuzzer pass to perform module donation (KhronosGroup#3117) Created with: roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools

Validator pass for image instructions

a886d2e

Includes validation rules for OpImageXXX and ImageOperand. Doesn't include OpTypeImage and OpImageSparseXXX.

Disabled invalid test

acc6480

atgoo requested review from antiagainst, dneto0 and ehsannas November 9, 2017 13:17

atgoo changed the title ~~WIP Validator pass for image instructions~~ Validator pass for image instructions Nov 9, 2017

dneto0 mentioned this pull request Nov 16, 2017

Validate OpTypeImage instruction #975

Closed

dneto0 requested changes Nov 16, 2017

View reviewed changes

KhronosGroup deleted a comment from dneto0 Nov 17, 2017

Review changes.

6ee0290

atgoo commented Nov 17, 2017

View reviewed changes

dneto0 approved these changes Nov 22, 2017

View reviewed changes

dneto0 closed this Nov 22, 2017

dneto0 mentioned this pull request Nov 22, 2017

Validator pass for OpImageXXX instructions #900

Open

Conversation

atgoo commented Nov 8, 2017

Uh oh!

atgoo commented Nov 8, 2017

Uh oh!

dneto0 left a comment

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!

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!

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!

atgoo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

atgoo commented Nov 17, 2017

Uh oh!

dneto0 left a comment

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!

dneto0 commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants