Page MenuHomePhabricator

[MLIR][SPIRVToLLVM] Implementation of spv.BitFieldInsert pattern
ClosedPublic

Authored by georgemitenkov on Jun 26 2020, 3:37 AM.

Details

Summary

This patch introduces conversion pattern for spv.BitFiledInsert op, as well as some utility functions to facilitate code reading. Since spv.BitFiledInsert may take both vector and integer operands, this case was specifically handled by broadcasting values (count and offset here) to vectors. Moreover, the types had to be converted to same bitwidth in order to conform with LLVM dialect rules. This was done with zext when extending (Note that count and offset are treated as unsigned) and trunc in the opposite case. For the latter one, truncation is safe since the op is defined only when count/offset/their sum is less than the bitwidth of the result. This introduces a natural bound of the value of 64, which can be expressed as i8.

Diff Detail

Event Timeline

georgemitenkov created this revision.Jun 26 2020, 3:37 AM
ftynse requested changes to this revision.Jun 26 2020, 7:52 AM
ftynse added inline comments.
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
87

Couldn't parse this sentence

89

Nit: this could use a more descriptive name, something like optionallyTruncateOrExtend. There are many other types of casting.

93

Would it be possible to factor this out into getLLVMTypeBitWidth(), and call it here and inside getValueBitWidth ?

118

Don't evaluate the upper bound on each iteration. We routinely use the idiom
for (unsigned i = 0, e = vectorType.getNumElements(); i < e; ++i)
for this reason.

Also, MLIR and LLVM overwhelmingly uses unsigned for sizes and loops around them

120

Can we cache the result of converting the type in a local variable?

194

I think this needs to replace the uses of op with the results of the newly created OrOp

This revision now requires changes to proceed.Jun 26 2020, 7:52 AM
georgemitenkov marked 6 inline comments as done.
  • [MLIR][SPIRVToLLVM] Addressed comments on BitFieldInsert pattern and utility functions
  • [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Improved code style: comments, full stops at the end and function names: e.g. optionallyCast to optionallyTruncateOrExtend.
Refactored duplicate code into utility (getLVMTypeBitWidth). Changed loop in nroadcast() to take unsigned as loop argument,
removed the call to vector size on each iteration, and put i32 type creation into a local variable. Added replaceOpWithNewOp()
so that all uses of the op are replaced.

Added missing initial commit.

antiagainst requested changes to this revision.Jun 29 2020, 5:29 PM

Awesome, LGTM. Just a few nits.

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
76

We can just use PatternRewriter here unless we need functionalities from the ConversionPatternRewriter subclass. This makes the function more applicable so later if you need to call it somewhere there is no need to potentially change again.

100

It seems this comment should not be placed here. This is a generic utility function and we can document its contract as "unconditionally truncate without checking the value". This comment should be placed at the call site in bitfiled insert pattern.

110

Why not using the function return value for broadcasted?

154

Super nit: align Count with each

mlir/test/Conversion/SPIRVToLLVM/bitwise-ops-to-llvm.mlir
81

We don't really need this test and the following one. The purpose of tests are certainly not enumerate all possible combinations and there are many many cases. :) We just need to make sure different "dimensions" are covered here. Integer vs vector and bitwidth difference. Bitwidth difference is already covered in scalar cases. Here it's fine to omit for vector cases. Less code means less maintenance burden. What do you think?

This revision now requires changes to proceed.Jun 29 2020, 5:29 PM
antiagainst added inline comments.Jun 29 2020, 5:36 PM
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
100

Or actually make the comment of this function explicit that this is for bitfield ops, which I think is the case for the next patch.

georgemitenkov updated this revision to Diff 274422.EditedJun 30 2020, 4:44 AM
georgemitenkov marked 6 inline comments as done.
  • [MLIR][SPIRVToLLVM] Addressed comments on BitFieldInsert pattern
  • [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Now createConstantAllBitsSet() and optionallyTruncateOrExtend() use PatternRewriter. The comment for optionallyTruncateOrExtend() was made more explicit that this is a utility for bit manipulation ops. Also, this function now returns a value.
Also, fixed a couple of nits.

Regarding the tests, since broadcast happens in a separate utility function, I think it will be actually fine to keep only one of the vector tests. Later for next patch, integer cases can also be reduced to the same width case (again, what is tested here is optinallyTruncateOrExtebd()).

ftynse accepted this revision.Jun 30 2020, 8:50 AM
antiagainst accepted this revision.Jul 2 2020, 9:17 AM
This revision is now accepted and ready to land.Jul 2 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Jul 6 2020, 6:45 PM
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
77

nit: Please add braces here, the body isn't really "trivial"

145

nit: this-> shouldn't be necessary unless you are inside of a template.

georgemitenkov marked 2 inline comments as done.Jul 14 2020, 9:19 AM