Page MenuHomePhabricator

[SPIRV][ImageOperand] Add Image Operands for Image Instructions (NFC)
ClosedPublic

Authored by Weiwei-2021 on Aug 20 2021, 5:41 PM.

Details

Summary

This patch is to add Image Operands in SPIR-V Dialect. And let ImageDrefGather to use Image Operands as an example.

Image Operands are used in many image instructions. The tricky part is that "Image Operands encodes what oprands follow, as per Image Operands".
And ususally, they are optional to image instructions.

I don't want to change the existing assemblyFormat too much. So this is the format I create for now:

%0 = spv.ImageDrefGather %1, ... %3 : f32 ["Bias|Lod"](%4, %5 : f32, f32) -> ...

Any comment about the Image Operands is welcome since it will impact almost all image instructions' format. Thank you in advance.

Anoter thing is that Image Operands have a lot of validation rules (See Section 3.14 in SPIR-V Spec). It would be a hugh amuont of work.
Therefore I leave an TODO in verifyImageOperands function.

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

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Aug 20 2021, 5:41 PM
Weiwei-2021 requested review of this revision.Aug 20 2021, 5:41 PM
antiagainst requested changes to this revision.Aug 23 2021, 8:28 AM

I guess you are sending this out for early comments. ODS and assembly LGTM. Need to have some minimal verification and (de)serialization to land though. :) Some more comments following.

Image Operands are used in many image instructions. The tricky part is that "Image Operands encodes what oprands follow, as per Image Operands".
And ususally, they are optional to image instructions.

Indeed. Using

OptionalAttr<SPV_ImageOperandAttr>:$imageoperands,
Variadic<SPV_Type>:$values

Is a good approach to me.

I don't want to change the existing assemblyFormat too much. So this is the format I create for now:
%0 = spv.ImageDrefGather %1, ... %3 : f32 ["Bias|Lod"](%4, %5 : f32, f32) -> ...
Any comment about the Image Operands is welcome since it will impact almost all image instructions' format. Thank you in advance.

This LGTM!

Another thing is that Image Operands have a lot of validation rules (See Section 3.14 in SPIR-V Spec). It would be a huge amount of work.

Therefore I leave an TODO in verifyImageOperands function.

It's fine to implement them piece by piece. Like we can support those image operands one by one; erroring out for the rest.

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

Nit: values is too generic. What about calling it operand_arguments?

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

This isn't necessary?

342

If attr is null but values isn't empty, that should be a validation error?

345

Nit: In MLIR we prefer early return when it simplifies code. So here if (!attr) return success().

348

FYI: we have bitEnumContains utility functions generated for probing whether a bit is set.

351

I assume this part needs to be rewritten? Not sure I follow what's verified here..

This revision now requires changes to proceed.Aug 23 2021, 8:28 AM
antiagainst added inline comments.Aug 23 2021, 8:30 AM
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
348

And also operator| and operator&.

Weiwei-2021 marked 5 inline comments as done.

Updated the verify part.

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

Sorry about the confusion. Initially, I want to check the "Bias" and "Lod" will have one following operand for each. And after reading the detail, I realized that they cannot show up at the same time. :(
Many image operands has explicit requirements on image instruction using it. And we don't even support those instructions. Hence I add an llvm_unreachable for all non-supported image operands in the updated patch.

fix some problems.

Weiwei-2021 marked 2 inline comments as done.Aug 24 2021, 7:54 PM
Weiwei-2021 added inline comments.
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
348

Thanks for pointing it out. :)

Weiwei-2021 added inline comments.Aug 30 2021, 8:00 AM
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
351

Ping @antiagainst to check whether this versino is ok.

antiagainst accepted this revision.Aug 31 2021, 5:12 PM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
345

This else isn't necesary.

This revision is now accepted and ready to land.Aug 31 2021, 5:12 PM
This revision was landed with ongoing or failed builds.Sep 1 2021, 1:14 PM
This revision was automatically updated to reflect the committed changes.