This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add casting ops to/from generic storage space
ClosedPublic

Authored by nirvedhmeshram on Sep 19 2022, 1:03 PM.

Diff Detail

Event Timeline

nirvedhmeshram created this revision.Sep 19 2022, 1:03 PM
nirvedhmeshram requested review of this revision.Sep 19 2022, 1:03 PM
antiagainst requested changes to this revision.Sep 19 2022, 3:38 PM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/IR/SPIRVCastOps.td
336

These ops should not derive from SPV_CastOp; they should derive from SPV_Op directly. (SPV_CastOp is meant for numeric value casting; here we are doing pointer casting.)

360
let arguments = (ins
  SPV_AnyPtr:$pointer
);

let results = (outs
  SPV_AnyPtr:$result
);

let assemblyFormat = [{
  $operand attr-dict `:` type($ptr) `to` type($result)
}];
362

This is missing availability spec (which says these ops depend on Kernel)?

Did you pull these op definitions in using https://github.com/llvm/llvm-project/blob/main/mlir/utils/spirv/define_inst.sh?

366

Similarly like the above op.

396

Similarly like the above op.

423

Note that for this op the spec requires two operands, one pointer, one storage. We have two choices:

  1. Define the op as what the spec requires---two operands. The storage operands kinda duplicates with the storage class in the result type. Pro is that we don't need to manually code the serialization/deserialization.
  2. Define the op as only having one operand. This would make the IR cleaner. It would require customized (de)serialization though.

My preference is 2. (You can find examples of how to write customized (de)serialization in the SerializeOps.cpp or DeseralizeOps.cpp.) But if you want to go with 1 i'm also fine with it.

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

This check isn't necessary if we have let arguments = (ins SPV_AnyPtr:$pointer) as said in the above.

1569

Same here.

1603

Same here.

1618

We need to additionally verify the storage operand matches the storage class in the result type (if defining the op as taking two operands).

This revision now requires changes to proceed.Sep 19 2022, 3:38 PM
nirvedhmeshram marked 10 inline comments as done.
nirvedhmeshram added inline comments.
mlir/include/mlir/Dialect/SPIRV/IR/SPIRVCastOps.td
423

Took a stab at option 2.

nirvedhmeshram marked an inline comment as done.Sep 20 2022, 2:03 PM
antiagainst accepted this revision.Sep 20 2022, 2:30 PM

Nice, thanks! I just have a few more nits. Feel free to land after addressing them. :)

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVCastOps.td
364

Please name this as $pointer to follow the spec.

Similarly for the other two ops.

mlir/lib/Target/SPIRV/Deserialization/DeserializeOps.cpp
552

Use opBuilder.create<spirv::GenericCastToPtrExplicitOp>(..) to avoid the explicit OperationState and string name?

This revision is now accepted and ready to land.Sep 20 2022, 2:30 PM
nirvedhmeshram marked 2 inline comments as done.Sep 20 2022, 3:00 PM