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

Add extraction gym's ILP extractor to test in nightly #738

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

oflatt
Copy link
Member

@oflatt oflatt commented Feb 17, 2025

Draft for now until successful nightly

@oflatt oflatt force-pushed the oflatt-ilp-extraction branch from a0a7f74 to 96ca9c1 Compare February 19, 2025 00:35
@oflatt oflatt marked this pull request as ready for review February 21, 2025 21:28
@@ -4,45 +4,32 @@ expression: visualization.result
---
# ARGS: 0
@main(v0: int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one looks suspicious. It seems the old one does loop unrolling and the new one does not.

@@ -14,18 +14,15 @@ expression: visualization.result
v7_: int = id c3_;
v8_: int = id c4_;
.b9_:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. The old one does loop unrolling (step by 4) and the new one step by 1.

The old one also does not do the optimization (x+2)+2 = x + 4

@@ -14,16 +14,15 @@ expression: visualization.result
v7_: int = id c3_;
v8_: int = id c4_;
.b9_:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as the sequential version

@@ -16,451 +16,25 @@ expression: visualization.result
v8_: bool = eq c3_ v0;
br v8_ .b9_ .b10_;
.b9_:
v11_: bool = eq c1_ c1_;
v11_: int = call @fac c1_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR seems to undo function inlining?

@@ -4,7 +4,7 @@ expression: visualization.result
---
# ARGS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems to increase the number of inlining

@@ -4,55 +4,32 @@ expression: visualization.result
---
# ARGS: 30 10
@main(v0: int, v1: int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrolling

@yihozhang
Copy link
Collaborator

I am leaning towards keeping this PR as a separate branch given (1) the snapshot has some big changes, (2) the extractor diff is non-trivial, (3) ILP figures we want are one-off, and (4) the deadline's in five days. What do you think?

@oflatt
Copy link
Member Author

oflatt commented Feb 24, 2025

This PR fixes Get nodes, as well as improving extraction performance by pruning the e-graph
I'll fix the known bug and see where we are

@oflatt
Copy link
Member Author

oflatt commented Feb 24, 2025

Ok let's keep in separate branch for now unless someone needs Get to work

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