This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Respect client API requirements for 64-bit index
ClosedPublic

Authored by antiagainst on Feb 25 2023, 10:24 PM.

Details

Summary

Vulkan requires GPU processor ID/count builtin variables to be
32-bit scalar or vector for all the cases. Similarly there
are special requirements for OpenCL. We need to make sure those
rules are respected when converting using 64bit for index.

Diff Detail

Event Timeline

antiagainst created this revision.Feb 25 2023, 10:24 PM
antiagainst requested review of this revision.Feb 25 2023, 10:24 PM
kuhar added inline comments.Feb 26 2023, 12:28 PM
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp
169–170

Could we run this unconditionally, either via a helper function or something like dim = createOrFold<UConvertOp>(loc, converter.getBuiltinIndexVector(), dim);?

antiagainst marked an inline comment as done.Feb 26 2023, 2:36 PM
antiagainst added inline comments.
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp
169–170

This is gated on two conditions to be true. I'm not sure how much it helps to introduce UConvert folder to only handle builtinType != indexType?

kuhar accepted this revision.Feb 26 2023, 3:12 PM
kuhar added inline comments.
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp
169–170

But it seems like the type converter should know about both of these conditions, so I'd expect it to pick the right type and decide if any conversions are necessary. Either directly within TypeConverter or as a helper that takes it as a parameter. Anyway, this seems like a detail, so feel free to leave this as-is if you like the current implementation.

This revision is now accepted and ready to land.Feb 26 2023, 3:12 PM
antiagainst marked 2 inline comments as done.Feb 26 2023, 10:12 PM
antiagainst added inline comments.
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp
169–170

Yeah, the type converter is right now only concerning types. What you expect is actually op-dependent type conversion, where you dynamically adjust converted-to type according to the current op. This is not supported yet. So we cannot have it done in the type converter right now..

This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.