Skip to content
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

[CIR][ThroughMLIR] lowering cir.bit.clz and cir.bit.ctz to MLIR #645

Merged
merged 3 commits into from
May 31, 2024

Conversation

Kritoooo
Copy link
Contributor

This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and test files.
I will complete the lowering of other cir.bit operations in subsequent PRs.

Comment on lines 428 to 438
matchAndRewrite(mlir::cir::BitCtzOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const override {
auto resultIntTy =
getTypeConverter()->convertType(op.getType()).cast<mlir::IntegerType>();
auto ctz = rewriter.create<mlir::math::CountTrailingZerosOp>(
op->getLoc(), adaptor.getInput());
auto newOp = createIntCast(rewriter, ctz->getResult(0), resultIntTy,
/*isSigned=*/false);
rewriter.replaceOp(op, newOp);
return mlir::LogicalResult::success();
}
Copy link
Member

@Lancern Lancern May 31, 2024

Choose a reason for hiding this comment

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

These two matchAndRewrite functions looks similar. Could you refactor out a common matchAndRewriteBitOp function to do the common job? You can reuse this function in later PRs when you try to lower other bit operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I will do that right away.

@Kritoooo Kritoooo requested a review from Lancern May 31, 2024 02:42
Copy link
Member

@Lancern Lancern left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for the review @Lancern

Added one minor nit

@@ -0,0 +1,94 @@
// RUN: cir-opt %s -cir-to-mlir -o %t.mlir
// RUN: FileCheck %s --input-file %t.mlir
Copy link
Member

Choose a reason for hiding this comment

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

In your next PRs can you please start writing these from source? You can use -fno-clangir-direct-lowering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll rewrite bit.cir to bit.c

@bcardosolopes bcardosolopes merged commit 5a0a234 into llvm:main May 31, 2024
6 checks passed
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
…#645)

This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and
test files.
I will complete the lowering of other `cir.bit` operations in subsequent
PRs.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
…#645)

This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and
test files.
I will complete the lowering of other `cir.bit` operations in subsequent
PRs.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…#645)

This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and
test files.
I will complete the lowering of other `cir.bit` operations in subsequent
PRs.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…#645)

This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and
test files.
I will complete the lowering of other `cir.bit` operations in subsequent
PRs.
lanza pushed a commit that referenced this pull request Nov 5, 2024
This pr adds cir.bit.clz and cir.bit.ctz lowering to MLIR passes and
test files.
I will complete the lowering of other `cir.bit` operations in subsequent
PRs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants