Page MenuHomePhabricator

[mlir][StandardToSPIRV] Fix signedness issue in bitwidth emulation.
ClosedPublic

Authored by hanchung on May 11 2020, 4:54 PM.

Details

Summary

Previously, after applying the mask, a negative number would convert to a
positive number because the sign flag was forgotten. This patch adds two more
shift operations to do the sign extension. This assumes that we're using two's
complement.

This patch applies sign extension unconditionally when loading a unspported integer width, and it relies the pattern to do the casting because the signedness semantic is carried by operator itself.

Diff Detail

Event Timeline

hanchung created this revision.May 11 2020, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 4:54 PM
hanchung added inline comments.May 11 2020, 5:11 PM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
704

It seems that we should check the signedness semantics of dstType?

This looks like we should apply following sign extension if srcType.isSignedInteger() returns true, and shouldn't apply it if srcType.isUnsignedInteger() returns true. In case of it is signless, ie srcType.isSignlessInteger() returns true, I don't know what should we do. It seems an undef to me.

hanchung updated this revision to Diff 264086.May 14 2020, 1:53 PM

Handle bitwidth emulation on unsigned operator.

hanchung retitled this revision from [mlir][StandardToSPIRV] Fix a bug in loading a non-i32 value. to [mlir][StandardToSPIRV] Fix signedness issue in bitwidth emulation..May 14 2020, 1:57 PM
hanchung edited the summary of this revision. (Show Details)
hanchung marked 3 inline comments as done.May 14 2020, 2:02 PM
hanchung added inline comments.
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
212–213

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?

704

See commit message for the explanation.

antiagainst requested changes to this revision.May 15 2020, 11:39 AM
antiagainst added inline comments.
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
158

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?

175

There are other ops like AtomicUMax? Maybe do some regex like def SPV_\w*U\w+Op inside include/Dialect/SPIRV/?

212–213

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(...).

This revision now requires changes to proceed.May 15 2020, 11:39 AM
hanchung updated this revision to Diff 264343.May 15 2020, 2:31 PM
hanchung marked 5 inline comments as done.

Add a TODO.

mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
158

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).

175

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?

212–213

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.

hanchung updated this revision to Diff 264393.May 15 2020, 4:58 PM
hanchung marked an inline comment as done.

Add check on more unsigned ops.

hanchung marked an inline comment as done.May 15 2020, 4:58 PM
hanchung added inline comments.
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
175

I list other ops as well.

antiagainst accepted this revision.May 18 2020, 12:58 PM
antiagainst marked an inline comment as done.

LGTM after addressing the last two comments. :)

mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
158

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.

159

Nit: have a ... trait

212–213

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?

This revision is now accepted and ready to land.May 18 2020, 12:58 PM
hanchung updated this revision to Diff 264736.May 18 2020, 3:44 PM
hanchung marked 5 inline comments as done.

address comments

hanchung added inline comments.May 18 2020, 3:44 PM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
158

Sounds good, I'll work on it. :)

212–213

This needs to pass -verify-diagnostics in the command and split the test out to another module. Does it look okay to you?

antiagainst accepted this revision.May 18 2020, 4:39 PM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.May 27 2020, 12:29 PM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
162

nit: static