Skip to content

Add CSE pass to Slinky - #375

Open
steven-johnson wants to merge 31 commits into
mainfrom
srj/cse
Open

Add CSE pass to Slinky#375
steven-johnson wants to merge 31 commits into
mainfrom
srj/cse

Conversation

@steven-johnson

Copy link
Copy Markdown
Collaborator

This is a pretty direct port of Halide's CSE code to Slinky. Opening as draft because I want to do more testing before landing, but it's ready for comment.

This is a pretty direct port of Halide's CSE code to Slinky. Opening as draft because I want to do more testing before landing, but it's ready for comment.
Comment thread builder/test/cse.cc Outdated
Comment thread builder/cse.cc Outdated
Comment thread builder/cse.cc Outdated
Comment thread builder/test/cse.cc Outdated
Comment thread builder/cse.cc Outdated
@steven-johnson
steven-johnson marked this pull request as ready for review May 31, 2024 18:40
Comment thread builder/test/cse.cc Outdated
Comment thread builder/cse.cc
Comment thread builder/test/cse.cc Outdated
Comment thread builder/cse.cc Outdated
dsharlet pushed a commit that referenced this pull request May 31, 2024
Fixes #374; also cherry-picks the fix for nested-let-expr and saniziting
`#` from #375
@steven-johnson

Copy link
Copy Markdown
Collaborator Author

I think this PR is in good state for what it actually does, but per offline discussion, it doesn't quite do enough in real-world work to pay for itself; it likely needs to be upgraded to handle cse'ing across Stmts where possible.

Comment thread builder/cse.cc Outdated
case intrinsic::semaphore_init:
case intrinsic::semaphore_signal:
case intrinsic::semaphore_wait: return false;
default: return true;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

free, trace_begin, and trace_end aren't pure either

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Cool, will add

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It would be better to flip this around and return true when the intrinsic is pure and false otherwise. That way, when someone adds an intrinsic, and they forget to mark it as pure, the failure is lack of CSE, rather than likely very hard to find bugs.

Comment thread builder/pipeline.cc Outdated

result = deshadow(result, ctx);
result = simplify(result);
result = common_subexpression_elimination(result, ctx);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think we should put this after optimize_symbols below.

@steven-johnson

Copy link
Copy Markdown
Collaborator Author

I reverted the change for now because the inefficient pass is indeed crazy-inefficient; we can't use a visitor to detect purity on-demand (it needs to be cached somehow).

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