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

Adding Bitand, Neg, lowbit tests, and lowbit optimizations #714

Merged
merged 20 commits into from
Feb 24, 2025
Merged

Conversation

FTRobbin
Copy link
Collaborator

@FTRobbin FTRobbin commented Jan 28, 2025

This PR adds two new operators Bitand and Neg, some tests, and analysis and optimization in Egglog that rewrite the naive loop implementation of the lowbit function into the Hacker's Delight one-liner.

This now outperforms llvm-O3-O3 by an order of magnitude on my machine because the closed form is an O(log n) speedup (n = 2 * 10^5).

The RVSDGs before and after the optimization:
lowbit_naive_br-rvsdg-conversion
lowbit_naive_br-rvsdg-optimize

Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

Looks great! Did you really want all of those to be benchmarks? Perhaps the fenwick tree should be a benchmark while your tests should be in tests/passing/small
We do more testing on the tests, including generating snapshots for the small folder

@FTRobbin FTRobbin changed the title Adding Bitand, Neg, and lowbit tests Adding Bitand, Neg, lowbit tests, and lowbit optimizations Feb 7, 2025
@oflatt
Copy link
Member

oflatt commented Feb 7, 2025

Will merge after successful nightly! Being careful right now because it seems like eggcc might have crashed the nightly last night...

;; PopcountIterations guarantees termination for non-zero values
;; lowbit(0) is undefined behavior

(rule (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This technically violates the weak linearity because we have not provided an equivalent value for other values of the then branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that if you are computing something else in your loop, you will not be able to extract without the loop.

Copy link
Member

Choose a reason for hiding this comment

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

Yihong is right- we need to provide a loop that doesn't have the state edge in it as an alternative. See the existing state edge passthrough file for an example of how it's done for if statements.

If you are confused why we have to do this, hopefully the paper clears it up! Good chance to read that section

Copy link
Member

Choose a reason for hiding this comment

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

Also, move this rule to that file and ruleset

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have fixed this issue, but I feel it makes more sense for the rule to stay in this file.

Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

Main concern is the state edge passthrough rule

;; PopcountIterations guarantees termination for non-zero values
;; lowbit(0) is undefined behavior

(rule (
Copy link
Member

Choose a reason for hiding this comment

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

Yihong is right- we need to provide a loop that doesn't have the state edge in it as an alternative. See the existing state edge passthrough file for an example of how it's done for if statements.

If you are confused why we have to do this, hopefully the paper clears it up! Good chance to read that section

;; PopcountIterations guarantees termination for non-zero values
;; lowbit(0) is undefined behavior

(rule (
Copy link
Member

Choose a reason for hiding this comment

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

Also, move this rule to that file and ruleset

Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

Awesome work! Small note about TmpCtx, not blocking but would be nice. Waiting on nightly to merge this

(let newlpinputs (TupleRemoveAt lpinputs j))
(let newpred_outputs (TupleRemoveAt pred_outputs (+ j 1)))

(let newlpctx (DummyLoopContext newlpinputs newpred_outputs pred_outputs))
Copy link
Member

Choose a reason for hiding this comment

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

These days we've decided to use TmpCtx instead, then make sure to delete it at the end of the rule. It's safer in case you forget to include some important information in the dummy context

@oflatt
Copy link
Member

oflatt commented Feb 21, 2025

Nightly looks great!

@oflatt oflatt merged commit 4202769 into main Feb 24, 2025
4 checks passed
@oflatt oflatt deleted the haobin_hacker branch February 24, 2025 15:42
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