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

[Han-Chung Wang via Phabricator via llvm-commits]

hanchung added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:158
 
+/// Returns true if the operator is operating on unsigned integers.
+template <typename SPIRVOp>
----------------
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).


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:175
+CHECK_UNSIGNED_OP(spirv::UGreaterThanOp);
+CHECK_UNSIGNED_OP(spirv::UGreaterThanEqualOp);
+
----------------
antiagainst wrote:
> There are other ops like `AtomicUMax`? Maybe do some regex like `def SPV_\w*U\w+Op` inside `include/Dialect/SPIRV/`?
Yes, there are other ops. I only list the ops I found in this file. I'm not sure if we can do regex on macro definition. I checked in the doc, and couldn't find it. Let's handle it with TreatOperandsAsUnsignedInteger approach?

Would you like me to list other ops that we could find in include/Dialect/SPIRV here? 


================
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");
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79753