[PATCH] D79753: [mlir][StandardToSPIRV] Fix signedness issue in bitwidth emulation.

Lei Zhang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 18 13:01:00 PDT 2020


antiagainst accepted this revision.
antiagainst marked an inline comment as done.
antiagainst added a comment.
This revision is now accepted and ready to land.

LGTM after addressing the last two comments. :)



================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:158
 
+/// Returns true if the operator is operating on unsigned integers.
+template <typename SPIRVOp>
----------------
hanchung wrote:
> antiagainst wrote:
> > I think we should have a `TreatOperandsAsUnsignedInteger` trait or something in the SPIR-V dialect so we can bake this information to the ops themselves. It's better structure: op semantics should belong to the op and it's better for reuse. Fine for me if you put a TODO here for now. But could you follow up on this later?
> SG and yes, I'm happy to follow up on this. Let's do it later so we can make some progress (and I also need some time to study it).
Yup! You can find examples for dialect specific traits in Linalg dialect: https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h#L29 and the wrapper in TableGen like https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td#L24. We can define something similar for the SPIR-V dialect.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:203-204
+    if (isUnsignedOp<SPIRVOp>() && dstType != operation.getType()) {
+      operation.emitError(
+          "bitwidth emunation is not implemented yet on unsigned op");
+    }
----------------
hanchung wrote:
> antiagainst wrote:
> > hanchung wrote:
> > > I'd like to return a failure with this statement, but it would cause an error and I don't know how to fix it in the test.
> > > 
> > > ```
> > > 'std.divi_unsigned' op requires the same type for all operands and results
> > > 
> > >  note: see current operation: %0 = "std.divi_unsigned"(%arg0, %arg1) : (i32, i32) -> i8
> > > ```
> > > 
> > > Does anyone have a better thought here?
> > This is because we are doing partial conversion in Standard to SPIR-V pass: https://github.com/llvm/llvm-project/blob/master/mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRVPass.cpp#L43. So the conversion for the wrapper function always succeeds and that will convert the types for the input for the op. For the pattern it bails out but the conversion for the wrapper function remains materialized so we are seeing the error.
> > 
> > You can try to change the pass to full conversion but there may exist other places assuming partial conversion so likely they need to be fixed. (If so it would be nice to have another patch for that.)
> > 
> > BTW, the more conventional way to report matching failures is via `return rewriter.notifyMatchFailure(...)`. 
> The failing tests are std-ops-to-spirv.mlir.test and std-types-to-spirv.mlir.test
> 
> I tried full conversion and also add mlir::ModuleOp and 
> mlir::ModuleTerminatorOp to legal op, but it still failing on legalizing func op. If I add func op to legal op, it crashes somewhere. I can fix it in a later change if I get a proper fix about it.
Ah, yes. Thanks for looking into this! It seems we should introduce a bool option to `-convert-std-to-spirv` to indicate whether to use partial or full conversion. Then we can split those tests (basically negative tests) that require a full conversion into a new file and test with `-convert-std-to-spirv=use-full-conversion` or something. For now I'm fine to `return operation.emitError(...)` here and check that the error message is `'std.divi_unsigned' op requires the same type for all operands and results`. Returning directly is safer than what's implemented at the moment, which we still carry on the op replacement. Could you change here?


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:159
+/// Returns true if the operator is operating on unsigned integers.
+/// TODO: Have a TreatOperandsAsUnsignedInteger and bake the information to the
+/// ops themselves.
----------------
Nit: have a ... trait


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79753/new/

https://reviews.llvm.org/D79753





More information about the llvm-commits mailing list