Page MenuHomePhabricator

[MLIR][SPIRVToLLVM] Implementation of spv.BitFieldSExtract and spv.BitFieldUExtract patterns
ClosedPublic

Authored by georgemitenkov on Fri, Jun 26, 3:47 AM.

Details

Summary

This patch adds conversion patterns for spv.BitFieldSExtract and spv.BitFieldUExtract. As in the patch for spv.BitFieldInsert, offset and count have to be broadcasted in vector case and casted to match the type of the base.

Diff Detail

Event Timeline

georgemitenkov created this revision.Fri, Jun 26, 3:47 AM

It doesn't seem like you are actually testing that these patterns replace the op.

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

nit: Drop trivial braces.

329

Where are you replacing op?

383

Same here.

georgemitenkov marked 3 inline comments as done.
  • [MLIR][SPIRVToLLVM] Addressed comments on style and op replacement
  • [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Removed trivial braces in conditional statement. Used replaceOpWithNewOp to replace the op.

antiagainst requested changes to this revision.Mon, Jun 29, 5:47 PM
antiagainst added inline comments.
mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp
283

The logic to broadcast and bitwidth-cast is the same for all there bitfield ops. Could we have a utility function to deduplicate them?

300

Nit: . to end.

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

Same as the previous patch. I think we can drop the bitwidth case permutation for vector cases.

This revision now requires changes to proceed.Mon, Jun 29, 5:47 PM
georgemitenkov marked 2 inline comments as done.
  • [MLIR][SPIRVToLLVM] Addressed style comments on bit manipulation patterns and removed unnecessary tests

Added a full stop in the comment. Reduced the number of vector tests to one. I suppose that for scalars it can also be the case? The functuinality is the same accross all 3 bitfiled ops. So we can have 3 scalar cases for spv.BitFieldInsert and 1 for extraction ops.
Broadcasting and truncating/extending will be moved to utility once BitFieldInsertPattern is merged.

  • [MLIR][SPIRVToLLVM] Addressed style comments on bit manipulation patterns and removed unnecessary tests

    Added a full stop in the comment. Reduced the number of vector tests to one. I suppose that for scalars it can also be the case? The functuinality is the same accross all 3 bitfiled ops. So we can have 3 scalar cases for spv.BitFieldInsert and 1 for extraction ops. Broadcasting and truncating/extending will be moved to utility once BitFieldInsertPattern is merged.

It is merged now. Please rebase and address issues. :)

antiagainst requested changes to this revision.Thu, Jul 2, 9:29 AM
This revision now requires changes to proceed.Thu, Jul 2, 9:29 AM
georgemitenkov updated this revision to Diff 275324.EditedFri, Jul 3, 1:28 AM
georgemitenkov marked an inline comment as done.
  • [MLIR][SPIRVToLLVM] Extracted common logic for broadcasting and extension/truncation for bit ops
  • [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Rebased against master. Extracted common functionality for bit manipulation ops to utilility functions. Now, both offset and count are processed via processCountOrOffset() function, that would call optionallyBroadcast() and optionallyTruncateOrExtend().
Also, processing offset and count is more logical now: offset first and then count (before the ordering was different for broadcasting and extension/truncation).

antiagainst accepted this revision.Tue, Jul 7, 6:38 PM

Nice! I believe you can land it by yourself now. So I won't spoil the fun here and will let you do it by yourself. :) Please make sure to follow https://llvm.org/docs/Phabricator.html#committing-a-change.

This revision is now accepted and ready to land.Tue, Jul 7, 6:38 PM
This revision was automatically updated to reflect the committed changes.

I see that the revision hasn't been landed due to changes picked up when rebasing against master (I think this is because I was committing via arc patch D*). Do I reopen the review in this case?