This is an archive of the discontinued LLVM Phabricator instance.

[mlir][StandardToSPIRV] Fix conversion for signed remainder
ClosedPublic

Authored by antiagainst on Jul 13 2020, 6:23 AM.

Details

Summary

Per the Vulkan's SPIR-V environment spec, "for the OpSRem and OpSMod
instructions, if either operand is negative the result is undefined."
So we cannot directly use spv.SRem/spv.SMod if either operand can be
negative. Emulate it via spv.UMod.

Because the emulation uses spv.SNegate, this commit also defines
spv.SNegate.

Diff Detail

Event Timeline

antiagainst created this revision.Jul 13 2020, 6:23 AM
hanchung added inline comments.Jul 13 2020, 9:43 AM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
341

Add a comment?

1065

Remove the extra space after "and".

s/patters/patterns

mlir/test/Dialect/SPIRV/arithmetic-ops.mlir
181

Let's follow naming style as others?

s/Snegate.../snegate...

rriddle added inline comments.Jul 13 2020, 10:19 AM
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
100

nit: Please use /// for top-level comments.

121

nit: Drop trivial braces here.

555

nit: Is the this-> necessary here?

Also can you spell out the type instead of using auto?

ThomasRaoux accepted this revision.Jul 13 2020, 10:59 AM
ThomasRaoux added inline comments.
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
118

Should we change this one to UModOp to avoid confusion?

556

Do we need that? The dstType is not used and if we create an illegal type we would fail later on?

This revision is now accepted and ready to land.Jul 13 2020, 10:59 AM
antiagainst marked 11 inline comments as done.

Address comments

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

Good catch!

Actually technically this should be okay here. Note that Vulkan's SPIR-V environment does not prohibit using these two opcodes; it's just that they cannot take on negative operands. Actually the spec is quite explicit regarding this: "While the OpSRem and OpSMod instructions are supported by the Vulkan environment, they require non-negative values and thus do not enable additional functionality beyond what OpUMod provides."

This is used by converting std.load/std.store; their indices should not be negative given they are of index type. So I think it's fine here. But agreed making it spv.UMod and be explicit in the comment is an improvement. Will have a follow-up patch for it.

555

Ah good catch. Bad copy-pasta.

556

Yes it's not needed. Leftover after copying. :)

This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.Jul 13 2020, 1:24 PM
antiagainst added inline comments.
mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
118