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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added inline comments.
This revision now requires changes to proceed.


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


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:175
+CHECK_UNSIGNED_OP(spirv::UGreaterThanOp);
+CHECK_UNSIGNED_OP(spirv::UGreaterThanEqualOp);
+
----------------
There are other ops like `AtomicUMax`? Maybe do some regex like `def SPV_\w*U\w+Op` inside `include/Dialect/SPIRV/`?


================
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:
> 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(...)`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79753