Skip to content

[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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIROps.td
Original file line number Diff line number Diff line change
Expand Up @@ -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]>> {
Copy link
Contributor

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 the b very unclear. Can we maybe spell it out, bit.byte_swap?

Copy link
Contributor

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, like bitreverse (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 like complex. or libc. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason, I’d like to eliminate the bit. prefix entirely and use the builtin names directly, like bitreverse (This can be, and probably should be done as separate PR).

I'll send a separate PR for this after this PR lands.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe spell it out, bit.byte_swap?

Updated.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 cir.bitreverse and cir.bswap. Is that correct? If so, that makes sense to me, and I don't see a reason to merge these with the .bit prefix only to remove it in a subsequent change when the prefix is removed from other bit operations.

```
}];
}

//===----------------------------------------------------------------------===//
// Assume Operations
//===----------------------------------------------------------------------===//
Expand Down
36 changes: 25 additions & 11 deletions clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,23 @@ static RValue emitBuiltinBitOp(CIRGenFunction &cgf, const CallExpr *e,
RValue CIRGenFunction::emitBuiltinExpr(const GlobalDecl &gd, unsigned builtinID,
const CallExpr *e,
ReturnValueSlot returnValue) {
mlir::Location loc = getLoc(e->getSourceRange());

// See if we can constant fold this builtin. If so, don't emit it at all.
// TODO: Extend this handling to all builtin calls that we can constant-fold.
Expr::EvalResult result;
if (e->isPRValue() && e->EvaluateAsRValue(result, cgm.getASTContext()) &&
!result.hasSideEffects()) {
if (result.Val.isInt()) {
return RValue::get(builder.getConstInt(getLoc(e->getSourceRange()),
result.Val.getInt()));
}
if (result.Val.isInt())
return RValue::get(builder.getConstInt(loc, result.Val.getInt()));
if (result.Val.isFloat()) {
// Note: we are using result type of CallExpr to determine the type of
// the constant. Classic codegen uses the result value to determine the
// type. We feel it should be Ok to use expression type because it is
// hard to imagine a builtin function evaluates to a value that
// over/underflows its own defined type.
mlir::Type type = convertType(e->getType());
return RValue::get(builder.getConstFP(getLoc(e->getExprLoc()), type,
result.Val.getFloat()));
return RValue::get(builder.getConstFP(loc, type, result.Val.getFloat()));
}
}

Expand All @@ -101,8 +100,6 @@ RValue CIRGenFunction::emitBuiltinExpr(const GlobalDecl &gd, unsigned builtinID,
assert(!cir::MissingFeatures::builtinCallMathErrno());
assert(!cir::MissingFeatures::builtinCall());

mlir::Location loc = getLoc(e->getExprLoc());

switch (builtinIDIfNoAsmLabel) {
default:
break;
Expand Down Expand Up @@ -185,11 +182,28 @@ RValue CIRGenFunction::emitBuiltinExpr(const GlobalDecl &gd, unsigned builtinID,
probability);
}

auto result = builder.create<cir::ExpectOp>(getLoc(e->getSourceRange()),
argValue.getType(), argValue,
expectedValue, probAttr);
auto result = builder.create<cir::ExpectOp>(
loc, argValue.getType(), argValue, expectedValue, probAttr);
return RValue::get(result);
}

case Builtin::BI__builtin_bswap16:
case Builtin::BI__builtin_bswap32:
case Builtin::BI__builtin_bswap64:
case Builtin::BI_byteswap_ushort:
case Builtin::BI_byteswap_ulong:
case Builtin::BI_byteswap_uint64: {
mlir::Value arg = emitScalarExpr(e->getArg(0));
return RValue::get(builder.create<cir::ByteSwapOp>(loc, arg));
}

case Builtin::BI__builtin_bitreverse8:
case Builtin::BI__builtin_bitreverse16:
case Builtin::BI__builtin_bitreverse32:
case Builtin::BI__builtin_bitreverse64: {
mlir::Value arg = emitScalarExpr(e->getArg(0));
return RValue::get(builder.create<cir::BitReverseOp>(loc, arg));
}
}

cgm.errorNYI(e->getSourceRange(), "unimplemented builtin call");
Expand Down
16 changes: 16 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,13 @@ mlir::LogicalResult CIRToLLVMBitPopcountOpLowering::matchAndRewrite(
return mlir::LogicalResult::success();
}

mlir::LogicalResult CIRToLLVMBitReverseOpLowering::matchAndRewrite(
cir::BitReverseOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
rewriter.replaceOpWithNewOp<mlir::LLVM::BitReverseOp>(op, adaptor.getInput());
return mlir::success();
}

mlir::LogicalResult CIRToLLVMBrCondOpLowering::matchAndRewrite(
cir::BrCondOp brOp, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
Expand All @@ -551,6 +558,13 @@ mlir::LogicalResult CIRToLLVMBrCondOpLowering::matchAndRewrite(
return mlir::success();
}

mlir::LogicalResult CIRToLLVMByteSwapOpLowering::matchAndRewrite(
cir::ByteSwapOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {
rewriter.replaceOpWithNewOp<mlir::LLVM::ByteSwapOp>(op, adaptor.getInput());
return mlir::LogicalResult::success();
}

mlir::Type CIRToLLVMCastOpLowering::convertTy(mlir::Type ty) const {
return getTypeConverter()->convertType(ty);
}
Expand Down Expand Up @@ -2035,8 +2049,10 @@ void ConvertCIRToLLVMPass::runOnOperation() {
CIRToLLVMBitCtzOpLowering,
CIRToLLVMBitParityOpLowering,
CIRToLLVMBitPopcountOpLowering,
CIRToLLVMBitReverseOpLowering,
CIRToLLVMBrCondOpLowering,
CIRToLLVMBrOpLowering,
CIRToLLVMByteSwapOpLowering,
CIRToLLVMCallOpLowering,
CIRToLLVMCmpOpLowering,
CIRToLLVMComplexCreateOpLowering,
Expand Down
20 changes: 20 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ class CIRToLLVMBitPopcountOpLowering
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMBitReverseOpLowering
: public mlir::OpConversionPattern<cir::BitReverseOp> {
public:
using mlir::OpConversionPattern<cir::BitReverseOp>::OpConversionPattern;

mlir::LogicalResult
matchAndRewrite(cir::BitReverseOp op, OpAdaptor,
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMBrCondOpLowering
: public mlir::OpConversionPattern<cir::BrCondOp> {
public:
Expand All @@ -104,6 +114,16 @@ class CIRToLLVMBrCondOpLowering
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMByteSwapOpLowering
: public mlir::OpConversionPattern<cir::ByteSwapOp> {
public:
using mlir::OpConversionPattern<cir::ByteSwapOp>::OpConversionPattern;

mlir::LogicalResult
matchAndRewrite(cir::ByteSwapOp op, OpAdaptor,
mlir::ConversionPatternRewriter &) const override;
};

class CIRToLLVMCastOpLowering : public mlir::OpConversionPattern<cir::CastOp> {
mlir::DataLayout const &dataLayout;

Expand Down
91 changes: 91 additions & 0 deletions clang/test/CIR/CodeGen/builtin_bit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,94 @@ int test_builtin_popcountg(unsigned x) {

// OGCG-LABEL: _Z22test_builtin_popcountgj
// OGCG: %{{.+}} = call i32 @llvm.ctpop.i32(i32 %{{.+}})

unsigned char test_builtin_bitreverse8(unsigned char x) {
return __builtin_bitreverse8(x);
}

// CIR-LABEL: @_Z24test_builtin_bitreverse8h
// CIR: %{{.+}} = cir.bit.reverse(%{{.+}} : !u8i) : !u8i

// LLVM-LABEL: @_Z24test_builtin_bitreverse8h
// LLVM: %{{.+}} = call i8 @llvm.bitreverse.i8(i8 %{{.+}})

// OGCG-LABEL: @_Z24test_builtin_bitreverse8h
// OGCG: %{{.+}} = call i8 @llvm.bitreverse.i8(i8 %{{.+}})

unsigned short test_builtin_bitreverse16(unsigned short x) {
return __builtin_bitreverse16(x);
}

// CIR-LABEL: @_Z25test_builtin_bitreverse16t
// CIR: %{{.+}} = cir.bit.reverse(%{{.+}} : !u16i) : !u16i

// LLVM-LABEL: @_Z25test_builtin_bitreverse16t
// LLVM: %{{.+}} = call i16 @llvm.bitreverse.i16(i16 %{{.+}})

// OGCG-LABEL: @_Z25test_builtin_bitreverse16t
// OGCG: %{{.+}} = call i16 @llvm.bitreverse.i16(i16 %{{.+}})

unsigned test_builtin_bitreverse32(unsigned x) {
return __builtin_bitreverse32(x);
}

// CIR-LABEL: @_Z25test_builtin_bitreverse32j
// CIR: %{{.+}} = cir.bit.reverse(%{{.+}} : !u32i) : !u32i

// LLVM-LABEL: @_Z25test_builtin_bitreverse32j
// LLVM: %{{.+}} = call i32 @llvm.bitreverse.i32(i32 %{{.+}})

// OGCG-LABEL: @_Z25test_builtin_bitreverse32j
// OGCG: %{{.+}} = call i32 @llvm.bitreverse.i32(i32 %{{.+}})

unsigned long long test_builtin_bitreverse64(unsigned long long x) {
return __builtin_bitreverse64(x);
}

// CIR-LABEL: @_Z25test_builtin_bitreverse64y
// CIR: %{{.+}} = cir.bit.reverse(%{{.+}} : !u64i) : !u64i

// LLVM-LABEL: @_Z25test_builtin_bitreverse64y
// LLVM: %{{.+}} = call i64 @llvm.bitreverse.i64(i64 %{{.+}})

// OGCG-LABEL: @_Z25test_builtin_bitreverse64y
// OGCG: %{{.+}} = call i64 @llvm.bitreverse.i64(i64 %{{.+}})

unsigned short test_builtin_bswap16(unsigned short x) {
return __builtin_bswap16(x);
}

// CIR-LABEL: @_Z20test_builtin_bswap16t
// CIR: %{{.+}} = cir.bit.bswap(%{{.+}} : !u16i) : !u16i

// LLVM-LABEL: @_Z20test_builtin_bswap16t
// LLVM: %{{.+}} = call i16 @llvm.bswap.i16(i16 %{{.+}})

// OGCG-LABEL: @_Z20test_builtin_bswap16t
// OGCG: %{{.+}} = call i16 @llvm.bswap.i16(i16 %{{.+}})

unsigned test_builtin_bswap32(unsigned x) {
return __builtin_bswap32(x);
}

// CIR-LABEL: @_Z20test_builtin_bswap32j
// CIR: %{{.+}} = cir.bit.bswap(%{{.+}} : !u32i) : !u32i

// LLVM-LABEL: @_Z20test_builtin_bswap32j
// LLVM: %{{.+}} = call i32 @llvm.bswap.i32(i32 %{{.+}})

// OGCG-LABEL: @_Z20test_builtin_bswap32j
// OGCG: %{{.+}} = call i32 @llvm.bswap.i32(i32 %{{.+}})

unsigned long long test_builtin_bswap64(unsigned long long x) {
return __builtin_bswap64(x);
}

// CIR-LABEL: @_Z20test_builtin_bswap64y
// CIR: %{{.+}} = cir.bit.bswap(%{{.+}} : !u64i) : !u64i

// LLVM-LABEL: @_Z20test_builtin_bswap64y
// LLVM: %{{.+}} = call i64 @llvm.bswap.i64(i64 %{{.+}})

// OGCG-LABEL: @_Z20test_builtin_bswap64y
// OGCG: %{{.+}} = call i64 @llvm.bswap.i64(i64 %{{.+}})