This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add support for Ldexp operation in glsl extended instruction set
ClosedPublic

Authored by Weiwei-2021 on Feb 22 2021, 2:30 PM.

Details

Summary

Add support for Ldexp operation in glsl extended instruction set for SPIR-V dialect.

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

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Feb 22 2021, 2:30 PM
Weiwei-2021 requested review of this revision.Feb 22 2021, 2:30 PM

Forget to mention it in the commit msg (too quick to click the button:)). I am not 100% sure how to refer an operand in the error msg (e.g., x, exp in this case). Therefore, I keep the words the same as the SPIR-V Extended Instruction Set Spec.

antiagainst requested changes to this revision.Feb 23 2021, 3:54 PM

Nice, thanks for the contribution!

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

This does not match with the assembly format. Actually with assembly format defined you can omit this grammar.

1108

These need to be updated to match the assembly format.

1119

This should be named as y to match with the description?

1123

Could you define it as

attr-dict $x  `:` type($x) `,` $exp `:` type($exp) `->` type($result)

so that the type is directly associated with the operand? This makes it easy to read. (Right now that the majority of the ops aren't defined this way but I'm considering to refactoring them to use this scheme.)

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

Nit: just use Type instead of Ty. Two more characters makes things more readable and I would typically expect tab to complete it so you won't need to even type those two characters for most of the cases. :)

3597

This can be autogenerated by adding AllTypesMatch<["x", "result"]> to the trait. Typically we want to autogen when possible.

3601

What about

if (significantType.isa<FloatType>() != exponentType.isa<IntegerType>())
  return ldexpOp.emitOpError("operands must both be scalars or vectors");

auto getNumElements = [](Type type) -> unsigned {
  if (auto vectorType = type.dyn_cast<VectorType>())
    return vectorType.getNumElements;
  return 1;
};

if (getNumElements(...) != getNumElements(...))
  return ldexpOp.emitOpError("operands must have the same number of elements");

?

This revision now requires changes to proceed.Feb 23 2021, 3:54 PM

Forget to mention it in the commit msg (too quick to click the button:)). I am not 100% sure how to refer an operand in the error msg (e.g., x, exp in this case). Therefore, I keep the words the same as the SPIR-V Extended Instruction Set Spec.

Yeah using the symbols defined in ODS is the way to go.

Weiwei-2021 marked 7 inline comments as done.
Weiwei-2021 added inline comments.
mlir/include/mlir/Dialect/SPIRV/IR/SPIRVGLSLOps.td
1102

good to know. Thanks!

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

Thanks and good to know this tip. Do we have any document for things like this? My main reference is this link: https://mlir.llvm.org/docs/OpDefinitions/, but it is not that directly to get tips like this.

3601

LGTM, thanks for writing it.

antiagainst accepted this revision.Feb 24 2021, 10:06 AM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
3597

I typically directly read OpBase.td to discover them. Don't worry about not finding every one of them for now. You'll learn gradually. Code reviews are also for this purpose. :)

This revision is now accepted and ready to land.Feb 24 2021, 10:06 AM
This revision was landed with ongoing or failed builds.Feb 24 2021, 10:09 AM
This revision was automatically updated to reflect the committed changes.