-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[CIR] Add bit reverse and byte reverse operations #147200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2661,6 +2661,45 @@ def BitPopcountOp : CIR_BitOpBase<"bit.popcnt", | |
}]; | ||
} | ||
|
||
def BitReverseOp : CIR_BitOpBase<"bit.reverse", | ||
CIR_UIntOfWidths<[8, 16, 32, 64]>> { | ||
let summary = "Reverse the bit pattern of the operand integer"; | ||
let description = [{ | ||
The `cir.bit.reverse` operation reverses the bits of the operand integer. | ||
Its only argument must be of unsigned integer types of width 8, 16, 32, or | ||
64. | ||
|
||
This operation covers the C/C++ builtin function `__builtin_bitreverse`. | ||
|
||
Example: | ||
|
||
```mlir | ||
%1 = cir.bit.reverse(%0 : !u32i): !u32i | ||
``` | ||
}]; | ||
} | ||
|
||
def ByteSwapOp : CIR_BitOpBase<"bit.bswap", CIR_UIntOfWidths<[16, 32, 64]>> { | ||
let summary = "Reverse the bytes in the object representation of the operand"; | ||
let description = [{ | ||
The `cir.bswap` operation takes an integer as operand, reverse the bytes in | ||
the object representation of the operand integer, and returns the result. | ||
|
||
The operand integer must be an unsigned integer. Its widths must be either | ||
16, 32, or 64. | ||
|
||
Example: | ||
|
||
```mlir | ||
// %0 = 0x12345678 | ||
%0 = cir.const #cir.int<305419896> : !u32i | ||
|
||
// %1 should be 0x78563412 | ||
%1 = cir.bit.bswap(%0 : !u32i) : !u32i | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is out of sync now. @xlauko Based on your other comments, I got the impression that you would like these to be |
||
``` | ||
}]; | ||
} | ||
|
||
//===----------------------------------------------------------------------===// | ||
// Assume Operations | ||
//===----------------------------------------------------------------------===// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like
bit.bswap
. I know this is lumped in as a bit-manipulation operation, but it makes the referent of theb
very unclear. Can we maybe spell it out,bit.byte_swap
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here — I’d also prefer to keep the names as close as possible to Clang builtins. Using similar names reduces cognitive load and makes it easier to trace things through the entire pipeline.
For the same reason, I’d like to eliminate the
bit.
prefix entirely and use the builtin names directly, likebitreverse
(This can be, and probably should be done as separate PR). I tend to interpret dot-separated names as namespaces, so using them just for readability feels a bit off. Grouping things likecomplex.
orlibc.
makes sense, as they are in a way subdialects, but when the goal is simply improved readability, I’d lean toward using underscores instead in general.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send a separate PR for this after this PR lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.