This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][SPIRVToLLVM] Added spv.constant conversion pattern for scalar and vector types
ClosedPublic

Authored by georgemitenkov on Jul 1 2020, 12:45 AM.

Details

Summary

This patch introduces conversion pattern for spv.constant with scalar and vector types. There is a special case when the constant value is a signed/unsigned integer (vector of integers). Since LLVM dialect does not have signedness semantics, the types had to be converted to signless ints.

Diff Detail

Event Timeline

georgemitenkov created this revision.Jul 1 2020, 12:45 AM
ftynse added inline comments.Jul 1 2020, 3:55 AM
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
157

Is this-> necessary here?

194

Getting a sign-extended value for originally-unsigned type sounds dangerous

rriddle added inline comments.Jul 1 2020, 1:28 PM
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
146

Please format this.

172

nit: Drop the newline.

173

dyn_cast can return null. Please either check for null here or use cast instead.

175

nit: Please avoid using the attribute values whenever you possibly can. They have to be reconstructed, meaning locking the context per element. Use the APInt range instead if you know it is an integer, getValues<APInt>().

179

getIntegerAttr can't fail, so there is no need to check anything here.

184

Seems like what you really want here is to do something like:

dstElementsAttr = dstElementsAttr.mapValues(signlessType, [](APInt value) { return value; });

That would remove the need for the loop above.

194

Please just use the APInt methods instead.

georgemitenkov marked 9 inline comments as done.
  • [MLIR][SPIRVToLLVM] Addressed comments on constant conversion
  • [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Changed code style:

  • Ckang reformatted, removed extra newlines
  • Used mapValues() with a lambda instead of the loop
  • Removed sign extension for APInt values in favour of APInt method

Also used cast() instead of dyn_cast().

antiagainst accepted this revision.Jul 2 2020, 11:25 AM
This revision is now accepted and ready to land.Jul 2 2020, 11:25 AM
This revision was automatically updated to reflect the committed changes.