Page MenuHomePhabricator

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

Authored by Weiwei-2021 on Feb 11 2021, 10:40 AM.

Details

Summary

add support for FrexpStruct in glsl extended instruction set.

co-authored-by: Alan Liu <alanliu.yf@gmail.com>

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.

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Feb 11 2021, 10:40 AM
Weiwei-2021 requested review of this revision.Feb 11 2021, 10:40 AM
Weiwei-2021 edited the summary of this revision. (Show Details)
ergawy added inline comments.Feb 12 2021, 4:34 AM
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 requested changes to this revision.Feb 16 2021, 6:54 AM

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 ...

This revision now requires changes to proceed.Feb 16 2021, 6:54 AM
Weiwei-2021 marked 14 inline comments as done.

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 accepted this revision.Feb 17 2021, 5:59 AM
antiagainst added inline comments.
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.

This revision is now accepted and ready to land.Feb 17 2021, 5:59 AM
This revision was automatically updated to reflect the committed changes.