[spirv] Add binary arithmetic operations. #54

denis0x0D · 2019-07-30T10:05:34Z

Hello, this patch implements subset of arithmetic binary operations such as: OpIAdd, OpFAdd, OpISub, OpFSub, OpIMul, OpFDiv, OpFRem, OpFMod. The ops reuse same parser, printer and verifier, so I've created a base class for them - ArithmeticOp. Tests added to check serialization and deserialization. I did not add tests into ops.mlir file, because all ops are reusing the same implementation and FMullOp already has tests.

As far as I understood, other binary arithmetic operation such as OpUMod, OpUDiv, OpSDiv, OpSRem, OpSMod require to check signess of the spirv integer type, in this case IntegerTypeOp should be fully implemented, and this affects a serialization and deserialization. If it's ok, I can implement them in next patch.
@antiagainst can you please take a look.
Thanks.

antiagainst

This is nice, thanks @denis0x0D! I just have two small comments. :)

antiagainst · 2019-07-30T12:34:49Z

include/mlir/Dialect/SPIRV/SPIRVOps.td

@@ -252,7 +255,100 @@ def SPV_ExecutionModeOp : SPV_Op<"ExecutionMode", [ModuleOnly]> {

 // -----

-def SPV_FMulOp : SPV_Op<"FMul", [NoSideEffect, SameOperandsAndResultType]> {
+def SPV_FAddOp : SPV_ArithmeticOp<"FAdd", SPV_Float> {


This op should also be marked as Commutative. We should add another template parameter to SPV_ArithmeticOp to allow additional traits on certain ops. (You can use !listconcat(traits, [NoSideEffect, SameOperandsAndResultType]) to concatenate them.)

Thanks for mention this! Fixed.

antiagainst · 2019-07-30T12:35:14Z

include/mlir/Dialect/SPIRV/SPIRVOps.td

+
+// -----
+
+def SPV_FMulOp : SPV_ArithmeticOp<"FMul", SPV_Float> {


Same here. Also need to mark as Commutative.

Thanks, fixed.

antiagainst · 2019-07-30T12:35:40Z

include/mlir/Dialect/SPIRV/SPIRVOps.td

+
+// -----
+
+def SPV_IAddOp : SPV_ArithmeticOp<"IAdd", SPV_Integer> {


antiagainst · 2019-07-30T12:35:50Z

include/mlir/Dialect/SPIRV/SPIRVOps.td

+
+// -----
+
+def SPV_IMulOp : SPV_ArithmeticOp<"IMul", SPV_Integer> {


antiagainst · 2019-07-30T12:37:54Z

test/Dialect/SPIRV/Serialization/bin_ops.mlir

@@ -0,0 +1,61 @@
+// RUN: mlir-translate -serialize-spirv %s | mlir-translate -deserialize-spirv | FileCheck %s
+// CHECK:           func {{@.*}}([[ARG1:%.*]]: f32, [[ARG2:%.*]]: f32) {


No need to check the funcs; just need to check the arithmetic ops. Checks in the test should focus on what we are testing. Checking additional stuff will make the test difficult to maintain in the future (say if we change how func are printed then we need to update everywhere, even in this test case, which is not related to testing func anyway.)

Besides, we typically add the // CHECK to be directly above the line to be checked for easy contrast. :)

Thanks for the explanation, fixed.

antiagainst · 2019-07-30T12:54:07Z

Integer signedness is a common confusing point in SPIR-V, which even led a dedicated section in the spec to explain: https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#_unsigned_versus_signed_integers.

Quote the spec:

Note in both cases all signed and unsigned operations always work on unsigned types, and the semantics of operation come from the opcode.

So OpU* and OpS* instructions should not care about the signedness on OpTypeInt; those instructions themselves dictate the semantics.

denis0x0D · 2019-07-30T13:24:42Z

@antiagainst thanks a lot for review, updated regarding to your comments. Can you please take a look.

So OpU* and OpS* instructions should not care about the signedness on OpTypeInt; those instructions themselves dictate the semantics.

Thanks for the explanation, I was thinking OpU*, OpS* requires additional checks, because of this:

Result Type must be a scalar or vector of integer type, whose Signedness operand is 0.

Thanks.

antiagainst

Nice, thanks!

MaheshRavishankar · 2019-07-30T14:00:40Z

test/Dialect/SPIRV/Serialization/bin_ops.mlir

+func @spirv_bin_ops() -> () {
+  spv.module "Logical" "VulkanKHR" {
+   func @fmul(%arg0 : f32, %arg1 : f32) {
+      // CHECK: [[VALUE:%.*]] = spv.FMul [[ARG1:%.*]], [[ARG2:%.*]] : f32


Nit: This doesnt need to be [[..:%.*]]. Just use {{%.*}} since these values arent matched anywhere else. Same for everything below as well

Thanks, fixed!

MaheshRavishankar · 2019-07-30T14:02:14Z

include/mlir/Dialect/SPIRV/SPIRVOps.td

@@ -66,8 +66,11 @@ def SPV_AccessChainOp : SPV_Op<"AccessChain", [NoSideEffect]> {
    no remaining (unused) indexes.

     Each index in Indexes
+


Nit : Why the extra line here?

It's a autofix by define_inst.sh script, which fixes my previous mistake, I've removed those lines by hands.

MaheshRavishankar · 2019-07-30T14:05:53Z

Maybe a simple test in ops.mlir is useful. Don't need all the negative testing, since they use the same path, but just to make sure the operations are recognized by the parser. (They obviously are since the serialization/deserialization works, but that is using a different binary)

antiagainst · 2019-07-30T14:41:40Z

@denis0x0D:

In the SPIR-V dialect, ideally I'd prefer to go with the first way of thinking from the spec, i.e., only have OpTypeInt == 0. (The serialization and deserialization is using == 1 right now so that should be fixed.) The difficulty right now is 1) we want to support deserialization of real-world SPIR-V blobs, which may contain both OpTypeInt with signedness == 0 and == 1. One way to handle this is that since we don't guarantee exact round trippable for deserialization, we can just deserialize both of them to i32. Another way is actually to have both signed and unsigned version of integer type. Another difficulty is that 2) right now real-world divers sometimes rely on the integer signedness bit to do stuff. But it depends on the hardware and their drivers so it's not that clear too... And based on my understanding, it's more of a problem for the graphics pipeline. Given we are focusing on the compute side, I'd prefer to not have two separate types.

Add binary operations such as: OpIAdd, OpFAdd, OpISub, OpFSub, OpIMul, OpFDiv, OpFRem, OpFMod.

denis0x0D · 2019-07-30T15:06:55Z

@MaheshRavishankar thanks for review, updated and added some tests to ops.mlir.

antiagainst · 2019-07-30T15:16:12Z

Thanks for the explanation, I was thinking OpU*, OpS* requires additional checks

After changing serialization and deserialization to only interact with OpTypeInt with signedness == 0, this will be true always. :)

denis0x0D · 2019-07-30T15:16:14Z

@antiagainst thanks for the detailed explanations, seems clear to me.

Add binary operations such as: OpIAdd, OpFAdd, OpISub, OpFSub, OpIMul, OpFDiv, OpFRem, OpFMod. Closes #54 COPYBARA_INTEGRATE_REVIEW=tensorflow/mlir#54 from denis0x0D:sandbox/bin_ops ff3689c441a6a181dbe52c410562346993163013 PiperOrigin-RevId: 260734166

googlebot added the cla: yes label Jul 30, 2019

antiagainst suggested changes Jul 30, 2019

View reviewed changes

denis0x0D force-pushed the sandbox/bin_ops branch from 28e7040 to 3f3ad82 Compare July 30, 2019 13:16

antiagainst approved these changes Jul 30, 2019

View reviewed changes

antiagainst added kokoro:run ready to pull labels Jul 30, 2019

kokoro-team removed the kokoro:run label Jul 30, 2019

MaheshRavishankar reviewed Jul 30, 2019

View reviewed changes

denis0x0D force-pushed the sandbox/bin_ops branch 2 times, most recently from c4b9cf6 to ff3689c Compare July 30, 2019 15:03

[spirv] Add binary arithmetic operations.

ff3689c

Add binary operations such as: OpIAdd, OpFAdd, OpISub, OpFSub, OpIMul, OpFDiv, OpFRem, OpFMod.

antiagainst approved these changes Jul 30, 2019

View reviewed changes

mlir-copybara-bot closed this in f577e95 Jul 30, 2019

bondhugula mentioned this pull request Oct 11, 2019

IR printing (-print-ir-after-all) crash #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spirv] Add binary arithmetic operations. #54

[spirv] Add binary arithmetic operations. #54

denis0x0D commented Jul 30, 2019

antiagainst left a comment

antiagainst Jul 30, 2019

denis0x0D Jul 30, 2019

antiagainst Jul 30, 2019

denis0x0D Jul 30, 2019

antiagainst Jul 30, 2019

denis0x0D Jul 30, 2019

antiagainst Jul 30, 2019

denis0x0D Jul 30, 2019

antiagainst Jul 30, 2019

denis0x0D Jul 30, 2019

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019

antiagainst left a comment

MaheshRavishankar Jul 30, 2019 •

edited

denis0x0D Jul 30, 2019

MaheshRavishankar Jul 30, 2019

denis0x0D Jul 30, 2019

MaheshRavishankar commented Jul 30, 2019

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019


		// -----

		def SPV_FMulOp : SPV_ArithmeticOp<"FMul", SPV_Float> {


		// -----

		def SPV_IAddOp : SPV_ArithmeticOp<"IAdd", SPV_Integer> {


		// -----

		def SPV_IMulOp : SPV_ArithmeticOp<"IMul", SPV_Integer> {

		@@ -0,0 +1,61 @@
		// RUN: mlir-translate -serialize-spirv %s \| mlir-translate -deserialize-spirv \| FileCheck %s
		// CHECK: func {{@.}}([[ARG1:%.]]: f32, [[ARG2:%.*]]: f32) {

		@@ -66,8 +66,11 @@ def SPV_AccessChainOp : SPV_Op<"AccessChain", [NoSideEffect]> {
		no remaining (unused) indexes.

		Each index in Indexes

[spirv] Add binary arithmetic operations. #54

[spirv] Add binary arithmetic operations. #54

Conversation

denis0x0D commented Jul 30, 2019

antiagainst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019

antiagainst left a comment

Choose a reason for hiding this comment

MaheshRavishankar Jul 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaheshRavishankar commented Jul 30, 2019

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019

antiagainst commented Jul 30, 2019

denis0x0D commented Jul 30, 2019

MaheshRavishankar Jul 30, 2019 •

edited