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

transformations: Implement control-flow-hoist #3103

Merged
merged 11 commits into from
Aug 28, 2024

Conversation

PapyChacal
Copy link
Collaborator

@PapyChacal PapyChacal commented Aug 27, 2024

Stacked on:

A couple of splittable but minor changes:

  • Update on some dialects for required traits
  • Make AffineEx hashable (as all attributes are expected to be! I just randomly crashed some CSE I was playing around with otherwise)

Just a simple and naive pass hoisting everything from region branches.

Thinking a bit, I realized this is all I missed for some slightly tricky cases to work the way I want in MLIR; see filecheck for more!

In a nutshell, MLIR provides some nice bits already, like loop-invariant-code-motion (Get stuff out of loop) and control-flow-sink (Nest stuff in branches) which are locally okay and smart, but contradict sometimes leading to frustrating things like, to reduce it as much as possible:

func.func @nested_loop_invariant(%n : index) {
    %0 = arith.constant 0 : index
    %1 = arith.constant 1 : index
    %100 = arith.constant 100 : index
    scf.for %i = %0 to %100 step %1 {
        %cond = "test.op"() : () -> (i1)
        %thing = "scf.if"(%cond)  ({
            %n100 = arith.muli %n, %100 : index 
            scf.yield %n100 :index
        }, {
            scf.yield %n : index
        }) : (i1) -> index
        "test.op"(%thing) : (index) -> ()
        scf.yield
    }
    return
}

From the scf.if's perspective, one should really keep the arith.muli nested. What a waste if you compute it and don't take this branch!
Thus from the loop's perspective, it's not as easy as hoisting an operation from its body and is not done.

So this pass does a typically-bad thing to help get out of this kind of local-minima frustration. Just naively hoist everything out of the conditionals to expose the loop-invariant bits, and sink the rest back if desired!

It should also help if someone wants to go from branches to precomputation + select, as I remember was a cool shader trick some amount of years ago at least 😋

@PapyChacal PapyChacal added the transformations Changes or adds a transformatio label Aug 27, 2024
@PapyChacal PapyChacal self-assigned this Aug 27, 2024
@PapyChacal PapyChacal changed the base branch from main to emilien/speculatibility August 27, 2024 14:51
@PapyChacal PapyChacal marked this pull request as ready for review August 27, 2024 14:59
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.89%. Comparing base (a2ed14d) to head (8ebba1b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3103      +/-   ##
==========================================
- Coverage   89.91%   89.89%   -0.02%     
==========================================
  Files         419      420       +1     
  Lines       53070    53129      +59     
  Branches     8226     8237      +11     
==========================================
+ Hits        47718    47761      +43     
- Misses       4023     4037      +14     
- Partials     1329     1331       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from emilien/speculatibility to main August 28, 2024 10:47
@PapyChacal PapyChacal merged commit 8218f23 into main Aug 28, 2024
14 checks passed
@PapyChacal PapyChacal deleted the emilien/control-flow-hoist branch August 28, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transformations Changes or adds a transformatio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants