This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Convert `ub.poison` to `spirv.undef`
ClosedPublic

Authored by Hardcode84 on Jul 24 2023, 12:37 PM.

Details

Summary

SPIR-V doesn't have poison, but poison can be converted to undef.

Diff Detail

Event Timeline

Hardcode84 created this revision.Jul 24 2023, 12:37 PM
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Hardcode84 requested review of this revision.Jul 24 2023, 12:37 PM
kuhar added a comment.Jul 24 2023, 2:30 PM

Thanks for taking care of this, @Hardcode84!

mlir/include/mlir/Conversion/Passes.td
1064

Nit: 'Converts supported UB ops to SPIR-V dialect ops.'

mlir/lib/Conversion/UBToSPIRV/UBToSPIRV.cpp
26
40–42

Could you define this inline? There no benefit to outlining these matchAndRewirte functions. This may be inconsistent with the older SPIR-V passed, but we converged on in-line definition in newer ones.

43

Shouldn't we use the type from the adaptor, or are these the same? Also, if the adaptor is unused, the argument should be unnamed (, OpAdaptor, )

63–64
mlir/test/Conversion/UBToSPIRV/ub-to-spirv.mlir
16–17

Could you add one test with vector<4xi32> to document how the non-scalar case is handled?

Hardcode84 added inline comments.Jul 24 2023, 2:41 PM
mlir/lib/Conversion/UBToSPIRV/UBToSPIRV.cpp
43

I don't see return type on adaptor, and I want to access unconverted type anyway, to avoid converting user types, which can coincidentally also be converted to primitive types.

kuhar added inline comments.Jul 24 2023, 2:43 PM
mlir/lib/Conversion/UBToSPIRV/UBToSPIRV.cpp
43

SG, thanks for the explanation

Hardcode84 added inline comments.Jul 24 2023, 2:55 PM
mlir/test/Conversion/UBToSPIRV/ub-to-spirv.mlir
16–17

vector is not covered yet (will be rejected by isIntOrIndexOrFloat)

review comments

Hardcode84 marked 5 inline comments as done.Jul 24 2023, 3:00 PM
kuhar added inline comments.Jul 24 2023, 3:15 PM
mlir/test/Conversion/UBToSPIRV/ub-to-spirv.mlir
16–17

This doesn't prevent us from having a test with a TODO for unsupported types that checks that at least the code doesn't crash

kuhar accepted this revision.Jul 24 2023, 3:16 PM

LGTM % testcase for an unsupported type

This revision is now accepted and ready to land.Jul 24 2023, 3:16 PM

vector negative test

Hardcode84 marked an inline comment as done.Jul 24 2023, 3:19 PM
This revision was landed with ongoing or failed builds.Jul 24 2023, 3:30 PM
This revision was automatically updated to reflect the committed changes.