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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp | ||
---|---|---|
90 | Couldn't parse this sentence | |
92 | Nit: this could use a more descriptive name, something like optionallyTruncateOrExtend. There are many other types of casting. | |
96 | Would it be possible to factor this out into getLLVMTypeBitWidth(), and call it here and inside getValueBitWidth ? | |
121 | Don't evaluate the upper bound on each iteration. We routinely use the idiom Also, MLIR and LLVM overwhelmingly uses unsigned for sizes and loops around them | |
123 | 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 |
- [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.
Awesome, LGTM. Just a few nits.
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp | ||
---|---|---|
79 | 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. | |
103 | 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. | |
113 | 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? |
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp | ||
---|---|---|
103 | 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. |
- [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()).
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.