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

Thorin Inline Optimization #144

Merged
merged 41 commits into from
Nov 28, 2022
Merged

Conversation

NeuralCoder3
Copy link
Collaborator

@NeuralCoder3 NeuralCoder3 commented Nov 18, 2022

In this PR, we want to realize the inline construction of compilation pipelines.

Concept:

// In compilation dialect
.ax %compiler.pass: *; 

// In specific dialect spec.thorin
.ax %spec.peephole: %compiler.pass;

// In spec.cpp
passes[flags_t(Axiom::Base<peephole>)] = [](PassMan& man) { man.add<direct::DS2CPS>(); };

// In test.thorin
.lam .extern _compile (world/program/[]) -> pipeline/world/program/* = {
   .let pipe = initPipeline ();
   .let pass_man = initPassMan();
   .let peephole_man = extendPassMan(pass_man, %spec.peephole);
   .let peephole_pipeline = extendPipeline (pipe, passManPhase(peephole_man));
   .let finished_pipeline = extendPipeline (peephole_pipeline, %core.optim_phase);
   finished_pipeline
}

Alternatively to PassMan we could go one level higher to PipelineBuilder.
We can not go one level lower to the pass directly due to reflection restrictions requiring one level (or two) of indirection.

The optimization function would look for the _compile function and evaluate it or simulate it to construct a new Pipeline to be executed.
Additionally, the compile could provide more advanced combinators and even constructs to guide the evaluation/simulation of the pipeline.

@leissa
Copy link
Member

leissa commented Nov 19, 2022

Really cool :)

I haven't looked into the impl yet, but right off the bat a few questions/minor remarks:

  • We can not go one level lower to the pass directly due to reflection restrictions requiring one level (or two) of indirection.

    Just out of curiosity, what's the problem here?

  • Alternatively to PassMan we could go one level higher to PipelineBuilder.

    Maybe we can have a pass dialect where the whole pass infrastructure lives and a phase dialect - importing pass - where the phase infrastructure lives.

  • What is %core.optim_phase? Just a place holder for the builtin phases/passes?

  • Maybe, we can use a hungry functions to make building a pass man or pipeline a more pleasant venture. See bugfix: detetect recursive function types in axioms #146 and this sketch of a phase dialect.

  • Finally a stylistic remark. For functions we use snake_case, for types CamelCase:

    .lam .extern _compile (world: World) -> Pipeline = {
       .let pipe              = init_pipeline ();
       .let pass_man          = init_pass_man ();
       .let peephole_man      = extend_pass_man(pass_man, %spec.peephole);
       .let peephole_pipeline = extend_pipeline (pipe, PassManPhase(peephole_man));
       .let finished_pipeline = extend_pipeline (peephole_pipeline, %core.optim_phase);
       finished_pipeline
    }
    

@NeuralCoder3
Copy link
Collaborator Author

NeuralCoder3 commented Nov 19, 2022

I haven't looked into the impl yet

The composition and execution are not yet implemented. I just sketched the communication.

  • Just out of curiosity, what's the problem here?

The lower level would be to write the classes of the passes into the map.
This is not directly possible due to the reflection restrictions in C++.
Instead, the "normal" way would be to create factory instances. We could also inline it (as done currently) by passing the create/modifier functions around.

What is %core.optim_phase? Just a place holder for the builtin phases/passes?

I wanted to express that some dialects provide pre-defined phases/pipeline parts.
For instance, I would expect core to offer the standard optimizations bundled as a phase.

            man.add<PartialEval>();
            man.add<BetaRed>();
            auto er = man.add<EtaRed>();
            auto ee = man.add<EtaExp>(er);
            man.add<Scalerize>(ee);
            man.add<TailRecElim>(er);

@leissa
Copy link
Member

leissa commented Nov 21, 2022

The doxygen issue is probably doxygen/doxygen#9668. I think the best "fix" for now, would be to disable WARN_AS_ERROR for now :/

@NeuralCoder3 NeuralCoder3 marked this pull request as ready for review November 22, 2022 14:00
@fodinabor fodinabor mentioned this pull request Nov 24, 2022
Copy link
Member

@leissa leissa left a comment

Choose a reason for hiding this comment

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

Fantastic work 👍

Just some nitpicking here and there. On a more general remark: Can we directly remove the old PipelineBuilder stuff or what is hindering us from nuking this?

dialects/affine/affine.cpp Outdated Show resolved Hide resolved
dialects/compile/compile.cpp Show resolved Hide resolved
dialects/compile/compile.cpp Show resolved Hide resolved
dialects/compile/compile.h Outdated Show resolved Hide resolved
dialects/compile/compile.thorin Outdated Show resolved Hide resolved
thorin/pass/pipelinebuilder.h Show resolved Hide resolved
thorin/pass/pipelinebuilder.h Outdated Show resolved Hide resolved
thorin/pass/pipelinebuilder.h Outdated Show resolved Hide resolved
thorin/pass/pipelinebuilder.h Outdated Show resolved Hide resolved
thorin/phase/phase.h Outdated Show resolved Hide resolved
@NeuralCoder3
Copy link
Collaborator Author

On a more general remark: Can we directly remove the old PipelineBuilder stuff or what is hindering us from nuking this?

The PipelineBuilder itself is currently used by the new system.
We can and probably should cut it a bit down and resolve a few layers of indirection.
We need some structure to group passes together and to remember pass instances. The grouping could be handled by an inline-pass-man instance. But I am not sure how to best store pass instances.

To remove the PipelineBuilder from the other places (adding default passes in each dialect, building a priority queue, resolving it to a pipeline, ...), we need a good way to handle default compilation.
Maybe a _default_compile in the compile.thorin file as fallback for _compile?

dialects/mem/mem.thorin Outdated Show resolved Hide resolved
@leissa
Copy link
Member

leissa commented Nov 26, 2022

Until we have fixed the macos issue, I'd suggest to simply disable these tests entirely. If everything else is green, I think we can merge.

@leissa
Copy link
Member

leissa commented Nov 26, 2022

To remove the PipelineBuilder from the other places (adding default passes in each dialect, building a priority queue, resolving it to a pipeline, ...), we need a good way to handle default compilation. Maybe a _default_compile in the compile.thorin file as fallback for _compile?

I thought about adding a dialect opt or so that provides the default compilation pipeline for all dialects maintained by us. So the only thing you have to do is to .import opt; and that's it.

But let's postpone this after this is merged.

@NeuralCoder3
Copy link
Collaborator Author

To remove the PipelineBuilder from the other places (adding default passes in each dialect, building a priority queue, resolving it to a pipeline, ...), we need a good way to handle default compilation.
Until we have fixed the macos issue, I'd suggest to simply disable these tests entirely. If everything else is green, I think we can merge.

Currently, the tests fail due to 4cb5d7f because the test cases no longer know which optimizations should be performed. (Some fail, some crash, some diverge, and some crash and diverge)

I thought about adding a dialect opt or so that provides the default compilation pipeline for all dialects maintained by us. So the only thing you have to do is to .import opt; and that's it.
But let's postpone this after this is merged.

I will for now re-add the old compilation such that the inline optimization is completely additional.

@NeuralCoder3
Copy link
Collaborator Author

NeuralCoder3 commented Nov 28, 2022

Until we have fixed the macos issue, I'd suggest to simply disable these tests entirely. If everything else is green, I think we can merge.
But let's postpone this after this is merged.

This PR is then ready to be merged.
Note: Default Compilation will happen in default-compilation.

@leissa leissa merged commit 0309dfc into AnyDSL:master Nov 28, 2022
@NeuralCoder3 NeuralCoder3 deleted the inline-optimize branch December 2, 2022 12:32
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