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.

Sign up for GitHub

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

Jump to bottom

[Comb]: Canonicalize and(x,y) and or(x,y) when x and y are defined by opposite comparisons #6374

Merged
merged 1 commit into from
Nov 22, 2023
Merged

[Comb]: Canonicalize and(x,y) and or(x,y) when x and y are defined by opposite comparisons #6374

merged 1 commit into from
Nov 22, 2023

Conversation

devins2518
Copy link
Contributor

This is my first time working on the Comb dialect, so this might not be the most optimal fold implementation. This could be generalized to apply to ands and ors of more than 2 values, but I wasn't sure how to hook this up into the m_Complement matcher on line 776 (for AndOp). That would also have an average complexity of O(n^2) which isn't ideal.

darthscsi
darthscsi reviewed Nov 8, 2023
View reviewed changes
lib/Dialect/Comb/CombFolds.cpp Outdated
@@ -743,6 +743,11 @@ static bool canonicalizeLogicalCstWithConcat(Operation *logicalOp,
return true;
}

static bool canCombineOppositeCmpIntoConstant(ICmpOp lhs, ICmpOp rhs) {
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 only valid for 'bin' flagged ops. The bin flag marks the op as only caring about the binary truth-table (rather than the vhdl or verilog truth-tables)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, hadn't thought about that. I've added a check for that. This wouldn't affect the andOp or orOp itself, correct?

@darthscsi
Copy link
Contributor

That would also have an average complexity of O(n^2) which isn't ideal.

This is a not-uncommon problem with the variadic forms of these ops. If you had purely binary versions of these ops (like llvm), then you would have the same problem, it just is expressed as needing to search through the or-op tree to find these things.

@uenoku
Copy link
Member

uenoku commented Nov 17, 2023

To reduce the time complexity perhaps we can do some sort of caching. We can create a set for seen ICmp values (DenseSet<tuple<ICMPPredicate, Value, Value>>) and insert seen values. We can check if there is an opposite value by querying a key tuple<negated(predicate), lhs, rhs>.

I had a semifinished branch for the this canonicalization
https://github.com/llvm/circt/compare/dev/uenoku/cano-negate) so maybe you can refer (the branch is messy though 🙃 )

@devins2518 devins2518 force-pushed the comb_const_fold branch 3 times, most recently from a993e1b to 250268e Compare November 17, 2023 20:40
@devins2518
Copy link
Contributor Author

@uenoku Thanks for the review! I've updated this PR with a lot of influence from your branch, and I think this implementation should be a lot nicer. I did not include the seenValues/seenNegatedValues tracking because xor not folding seems to reliably fold to -1 which is handled by other and/or folds. I've included it in the test cases for this fold just to ensure that if this behavior ever changes in the future, it can be easily caught and added here if necessary.

uenoku
uenoku approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

lib/Dialect/Comb/CombFolds.cpp Outdated Show resolved Hide resolved
lib/Dialect/Comb/CombFolds.cpp Outdated Show resolved Hide resolved
lib/Dialect/Comb/CombFolds.cpp Outdated
@@ -827,6 +849,14 @@ OpFoldResult AndOp::fold(FoldAdaptor adaptor) {
}
}

// x0 = icmp(x, y)
// x1 = n_icmp(x, y)
Copy link
Member

Choose a reason for hiding this comment

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

nit: n_icmp might not be easy to understand at the first look so could you write more descriptive comment here like icmp(pred, x, y), icmp(nagated(pred), x, y)) -> 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to pred and !pred which hopefully should be clear enough.

@devins2518
[Comb]: Canonicalize and(x,y) and or(x,y) when x and y are defined by…
77b98dc 
… opposite comparisons

- and(x,y) -> 0
- or(x,y) -> 1
- Only affects `and` and `or` of operands of binary `ICmpOp` define to
  handle definite 0 or 1 cases

Resolves llvm#5729
@uenoku
Copy link
Member

uenoku commented Nov 22, 2023

Thanks!

@uenoku uenoku merged commit fc4596d into llvm:main Nov 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uenoku uenoku uenoku approved these changes

@darthscsi darthscsi Awaiting requested review from darthscsi darthscsi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@devins2518 @darthscsi @uenoku