Changeset View
Changeset View
Standalone View
Standalone View
mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
| Show First 20 Lines • Show All 3,528 Lines • ▼ Show 20 Lines | for (auto operand : enclosedOp.getOperands()) | ||||
| if (!isa<spirv::ConstantOp, spirv::ReferenceOfOp, | if (!isa<spirv::ConstantOp, spirv::ReferenceOfOp, | ||||
| spirv::SpecConstantOperationOp>(operand.getDefiningOp())) | spirv::SpecConstantOperationOp>(operand.getDefiningOp())) | ||||
| return constOp.emitOpError( | return constOp.emitOpError( | ||||
| "invalid operand, must be defined by a constant operation"); | "invalid operand, must be defined by a constant operation"); | ||||
| return success(); | return success(); | ||||
| } | } | ||||
| //===----------------------------------------------------------------------===// | |||||
| // spv.GLSL.FrexpStruct | |||||
| //===----------------------------------------------------------------------===// | |||||
| static LogicalResult | |||||
antiagainst: We could use
```
let assemblyFormat = "attr-dict $operand `:` type($operand) `->` type… | |||||
Thanks. Remove parse and print. Add in ODS instead. Weiwei-2021: Thanks. Remove parse and print. Add in ODS instead. | |||||
| verifyGLSLFrexpStructOp(spirv::GLSLFrexpStructOp frexpStructOp) { | |||||
| spirv::StructType structTy = | |||||
| frexpStructOp.result().getType().dyn_cast<spirv::StructType>(); | |||||
| if (structTy.getNumElements() != 2) | |||||
| return frexpStructOp.emitError("result type must be a struct type " | |||||
| "with two memebers"); | |||||
| Type significandTy = structTy.getElementType(0); | |||||
| Type exponentTy = structTy.getElementType(1); | |||||
| VectorType exponentVecTy = exponentTy.dyn_cast<VectorType>(); | |||||
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. :) antiagainst: s/SPIRV/SPIR-V/
SPIR-V is the official name. We cannot use it in the code but in comment or… | |||||
| IntegerType exponentIntTy = exponentTy.dyn_cast<IntegerType>(); | |||||
| Type operandTy = frexpStructOp.operand().getType(); | |||||
| VectorType operandVecTy = operandTy.dyn_cast<VectorType>(); | |||||
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; ergawy: nit: I think you mean to convey the negation of this message, something like:
```
(loc… | |||||
| FloatType operandFTy = operandTy.dyn_cast<FloatType>(); | |||||
| if (significandTy != operandTy) | |||||
| return frexpStructOp.emitError("member zero of the resulting struct type " | |||||
| "must be the same type as the operand"); | |||||
| if (exponentVecTy) { | |||||
| IntegerType componentIntTy = | |||||
| exponentVecTy.getElementType().dyn_cast<IntegerType>(); | |||||
| if (!(componentIntTy && componentIntTy.getWidth() == 32)) | |||||
| return frexpStructOp.emitError( | |||||
| "member one of the resulting struct type must" | |||||
| "be a scalar or vector of 32 bit integer type"); | |||||
| } else if (!(exponentIntTy && exponentIntTy.getWidth() == 32)) { | |||||
| return frexpStructOp.emitError( | |||||
| "member one of the resulting struct type " | |||||
| "must be a scalar or vector of 32 bit integer type"); | |||||
| } | |||||
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. antiagainst: No need to duplicate the check here given this is already guaranteed by `SPV_AnyStruct`. The… | |||||
| // Check that the two member types have the same number of components | |||||
| if (operandVecTy && exponentVecTy && | |||||
We need to make sure the struct has 2 members before accessing them. ergawy: We need to make sure the struct has 2 members before accessing them. | |||||
nit: Here and below, can we change the names from: memberZero... to significand... and memberOne... to exponent...? ergawy: nit: Here and below, can we change the names from: `memberZero...` to `significand...` and… | |||||
| (exponentVecTy.getNumElements() == operandVecTy.getNumElements())) | |||||
| return success(); | |||||
| if (operandFTy && exponentIntTy) | |||||
| return success(); | |||||
| return frexpStructOp.emitError( | |||||
| "member one of the resulting struct type " | |||||
| "must have the same number of components as the operand type"); | |||||
nit: No need for the comment, the condition and error message are both quite clear. ergawy: nit: No need for the comment, the condition and error message are both quite clear. | |||||
| } | |||||
Could you add a test case for this error? antiagainst: Could you add a test case for this error? | |||||
| namespace mlir { | namespace mlir { | ||||
... must be the same type ... antiagainst: ... must be the same type ... | |||||
| namespace spirv { | namespace spirv { | ||||
| // TableGen'erated operation interfaces for querying versions, extensions, and | // TableGen'erated operation interfaces for querying versions, extensions, and | ||||
| // capabilities. | // capabilities. | ||||
| #include "mlir/Dialect/SPIRV/IR/SPIRVAvailability.cpp.inc" | #include "mlir/Dialect/SPIRV/IR/SPIRVAvailability.cpp.inc" | ||||
| } // namespace spirv | } // namespace spirv | ||||
nit: The convention is not to use {} if there is a single statement in the scope. I consistently forget this too :D. ergawy: nit: The convention is not to use `{}` if there is a single statement in the scope. I… | |||||
| } // namespace mlir | } // namespace mlir | ||||
| // TablenGen'erated operation definitions. | // TablenGen'erated operation definitions. | ||||
| #define GET_OP_CLASSES | #define GET_OP_CLASSES | ||||
| #include "mlir/Dialect/SPIRV/IR/SPIRVOps.cpp.inc" | #include "mlir/Dialect/SPIRV/IR/SPIRVOps.cpp.inc" | ||||
| namespace mlir { | namespace mlir { | ||||
| namespace spirv { | namespace spirv { | ||||
| // TableGen'erated operation availability interface implementations. | // TableGen'erated operation availability interface implementations. | ||||
| #include "mlir/Dialect/SPIRV/IR/SPIRVOpAvailabilityImpl.inc" | #include "mlir/Dialect/SPIRV/IR/SPIRVOpAvailabilityImpl.inc" | ||||
| } // namespace spirv | } // namespace spirv | ||||
| } // namespace mlir | } // namespace mlir | ||||
We could use
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.