[mlir][spirv] Add support for FrexpStruct in GLSL extended instruction set

[Weiwei-2021]

Summary

add support for FrexpStruct in glsl extended instruction set.

co-authored-by: Alan Liu <[e-mail]>

FYI, I tried to use define_inst.sh to autogen FrexpStruct in this branch but cannot make work. It seems that this script only supports spirv core instructions. Alan did this part before, I will try to sync up with him on how he makes it. But whether wonder anyone else know how to do it.

[ergawy]

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

1021

Look like the description is taken from an old revision of the spec. Given that Frexp is not supported here (and deprecated), it would be great to use the latest revision: https://www.khronos.org/registry/spir-v/specs/unified1/GLSL.std.450.html

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp

3555

nit: I think you mean to convey the negation of this message, something like:

(loc, "expected result structure type to consist of two members, but provided") << type;

3576

We need to make sure the struct has 2 members before accessing them.

3576

nit: Here and below, can we change the names from: memberZero... to significand... and memberOne... to exponent...?

3585

nit: No need for the comment, the condition and error message are both quite clear.

3594

nit: The convention is not to use {} if there is a single statement in the scope. I consistently forget this too :D.

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

394

I think this test exercises 2 different error paths at once: (1) the argument is not a floating point type, and (2) the arity of the argument and the second struct member must match (i.e. either both scalars or both vectors). Can you split this to 2 different cases as such?

mlir/test/Target/SPIRV/glsl-ops.mlir

31

One more test for the vector case would also be great.

[antiagainst]

Nice, thanks for working on this!

FYI, I tried to use define_inst.sh to autogen FrexpStruct in this branch but cannot make work. It seems that this script only supports spirv core instructions. Alan did this part before, I will try to sync up with him on how he makes it. But whether wonder anyone else know how to do it.

Yeah I think the script is not extended to support GLSL ops yet. Would be great to make it work there. But it's also fine for me to just manually add for now given we have been doing that for a while.

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

1021

+1

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp

3540

We could use

let assemblyFormat = "attr-dict $operand `:` type($operand) `->` type($result)

in the ODS to autogenerate the parser and printer.

Here I see you are validating the result type. There is no need to do it here: validation should be deferred to the validator where possible, which is auto partially autogenerated by the ODS spec (like SPV_AnyStruct would guarantee that the result type is a struct). In general, only validation needed for parsing itself should be checked in the parser.

3551

s/SPIRV/SPIR-V/

SPIR-V is the official name. We cannot use it in the code but in comment or strings we should use the official name. :)

3573

No need to duplicate the check here given this is already guaranteed by SPV_AnyStruct. The manual validation only needs to cover what's not covered by ODS.

3587

Could you add a test case for this error?

3588

... must be the same type ...

[Weiwei-2021]

I tried to use define_inst.sh to create autogen GLSL

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

1021

Thanks for clarifying the generation of glsl extension. I was confused by the fact that all ops containing "<!-- End of AutoGen section-->". We added this instruction months ago, and I didn't notice the out-of-date problem. I will update with the latest version.

A side question is that if knronos update the spirv version's, those documents needs to get update or we haven't done things like this?

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp

3540

Thanks. Remove parse and print. Add in ODS instead.

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

394

Actually it is just a lazy copy and past. :)

mlir/test/Target/SPIRV/glsl-ops.mlir

31

Seems that this whole test only tests scalar type although those operations support vector. Let us leave it like this considering that the test in Dialect is checking the validation of vector types.

[antiagainst]

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

1021

A side question is that if knronos update the spirv version's, those documents needs to get update or we haven't done things like this?

Yeah. But I won't worry that too much. The spec is not updated very frequently. Changes will be reflected when we add new ops. There is a delay there but it's fine to me.