Skip to content

[CIR][CodeGen][Lower] CIR Unary Not Operation. #30

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

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

redbopo
Copy link
Contributor

@redbopo redbopo commented Feb 9, 2023

Add Unary Not operation on CIRGen and Lowerings.

Most follow the work like #20
Currently not add LNot operation, since it requires the type to be cir.bool.

@bcardosolopes
Copy link
Member

bcardosolopes commented Feb 15, 2023

This looks pretty good, thanks.

@@ -410,7 +410,21 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
return Visit(E->getSubExpr());
}

mlir::Value VisitUnaryNot(const UnaryOperator *E) { llvm_unreachable("NYI"); }
mlir::Value VisitUnaryNot(const UnaryOperator *E) {
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason on why not following LLVM's codegen? We usually try to match the skeleton as much as possible. Any reason why this wouldn't work?

Value *ScalarExprEmitter::VisitUnaryNot(const UnaryOperator *E) {
  TestAndClearIgnoreResultAssign();
  auto op = Visit(E->getSubExpr());
  // return buildUnaryOp ....
}

@bcardosolopes
Copy link
Member

Thanks for the update.

I'm getting a bit confused with the way you are working with commits, for instance I don't understand what the purpose of the second commit here. You can re-organize your commits (squash, etc) and force push after you address a review. To speed up reviews: please make sure that any PR you send only contains one commit, add that they should be self-contained with change + test, you can always break things into multiple PRs.

@redbopo
Copy link
Contributor Author

redbopo commented Feb 26, 2023

Thanks @bcardosolopes .
Usually I like to make multi commits on github if the following commits are concerned of the PR, or just small fix.
And next to use the native git squash merge provided by github.com to write these with a single commit.

I will fix with one commit one PR in clangir next.

  Add Unary Not Operation on CIRGen and Lowerings.
@bcardosolopes bcardosolopes merged commit 1e1a458 into llvm:main Mar 1, 2023
@redbopo redbopo deleted the unaryOpNotLower branch March 1, 2023 12:35
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.

2 participants