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

swap-ir-impl starting point #4

Open
wants to merge 38 commits into
base: feat-implement-ir
Choose a base branch
from
Open

Conversation

Soulthym
Copy link
Collaborator

@Soulthym Soulthym commented Dec 2, 2024

  • Define missing operation types: now under ir2/nodes/...

    • Leafs
      • Value
      • Variable Parameter
    • Binary ops
      • Add
      • Sub
      • Mul
    • Unary ops
      • Enf
      • Boundary
    • Aggregation
      • Vector
      • Matrix
    • Blocks
      • Function
      • Evaluator
      • For
      • If
    • Structured ops
      • Call
      • Fold
  • Implement Spanned trait for nodes:

#derive(Spanned)
struct ... {
  #[span]
  span: ...
  ...
}
  • Implement converters for:
Link<NodeType> -> Link<Node>
Link<NodeType> -> Link<Leaf<T>>
Link<NodeType> -> Link<Graph>

Helper traits are available (IsNode, NotNode, IsGraph, NotGraph, IsLeaf, NotLeaf)

  • And in reverse.

  • Match graph/mod.rs api, now under ir2/graph.rs

    • Pretty printer (for better SSA visualization): uncommited - waiting for all nodes to match
      - [ ] insert_op_* -> new_*().add_child() or Add::new(parent, lhs, rhs)
  • Redefine visitor pattern with the new structure

  • Redefine remaining APIs referencing NodeIndex, replace them with Link<NodeType>

  • Convert passes to use ir2 and updated visitor pattern

    • Lowering AST > MIR
    • Inlining
    • Unrolling
    • Lowering MIR > AIR
  • Add validation to the builder pattern

  • Maybe isolate builder pattern into its own trait

@bitwalker
Copy link
Collaborator

bitwalker commented Dec 4, 2024

@Soulthym I just realized that since this PR is in your fork, and not the 0xPolygonMiden repo, I can't leave review comments. Not sure if it's easier to open a PR stacked on 0xPolygonMiden/air-script#359 with these changes, or add me as a collaborator to this repo for reviews, but I'll have to wait for one or the other to provide my review feedback. Just let me know which!

@Soulthym
Copy link
Collaborator Author

Soulthym commented Dec 5, 2024

Hey @bitwalker, we just added you as a contributor to our fork. We hope that solves the issue.

Leo-Besancon and others added 10 commits December 10, 2024 08:59
* derive Hash for SpannedMirValue

* ir3: better node splitting, no macro, simpler api
missimg builder pattern

* replaced Link<Op> by Link<Owner> for Operations and Leaves

* added IndexAccess Operand

* renamed IndexAccess to Accessor
re use parser's AccessType
impl Hash for AccessType

* fix call being copied instead of pointed to by get_children()

* typesafe builder pattern for most ops. Misssing structured ops

* type-safe Builder pattern for structured ops

* MirGraph Builder editing

* Add Leaves to Op enum

* ir3: comment top level items

* isolate Leaf, Op, Owner, Root

* add converters to enums

* added converters to structs
renamed IndexAccess to Acccessor

* Initial merge ir3 > update_passes

* remove comments

* Update mod.rs

* Graph: implement public api

* Swapped in helpers of passes/mod.rs

* Node: all node types + converters

* Full inlining with Node + cargo fmt

* Remove Link for FoldOperator

* Swap unrolling

* Swap lowering Mir -> Air

* Update visitor (with suboptimal get_children() herlper, to refactor)

* Add MirType to Parameter

* Update inlining to handle evaluators

* most of translated adapted

* add type support for Parameter in translate

* in build_statement, handle Owner::None (integrity or boundary constraint)

---------

Co-authored-by: Thybault Alabarbe <[email protected]>
@Soulthym
Copy link
Collaborator Author

Soulthym commented Dec 12, 2024

Hey @bitwalker,

We have some tests passing on the end2end pipeline, missing the lowering pass ones.

Missing features:

  • tests for the lowering Mir -> Air
  • span fields
  • a rework of the visitor pattern specialized with the new structure
  • improve documentation
  • builder derive macro
  • evaluator argument mapping
  • rework the passes with the new visitor pattern.
  • conditional evaluators

Based on the mir crate tests, here is what is currently missing/broken:

  • Too restrictive handling of iterables during translation. Affects:
    • functions
    • integrity_constraints
    • list_folding
  • miden-diagnostics unwraps on File missing. I suspect it's due to the missing spans. Affects:
    • selectors
  • inlining does not mutate the Mir in all places.

Edit as of 13 dec.: all current tests fixed by relaxing constraints for iterables

Once the API is stabilized, we will refactor the boilerplate.

@Soulthym
Copy link
Collaborator Author

Soulthym commented Dec 20, 2024

Hey @bitwalker, here is a recap of the work left to do.

The latest version is available on a new branch of this fork: https://github.com/massalabs/air-script/tree/fix-ir-translate
It will get merged in the current branch once all issues are resolved.

Currently, the tests in the mir package (cargo test --package mir) translate from AST to MIR, and we've identified the problems detailed below in the obtained graphs. Otherwise the graphs seem consistent with what we expect.

There are currently 3 identified issues left to work on, plus the features not marked as resolved above.

Parameter/argument unpacking:

Parameter unpacking for *AccessBinding is currently done during translation from ast to mir, but should happen during inlining.

  • Don't unpack call arguments during build, and dont check the length of arguments.
  • Don't unpack parameters during translation from ast to Mir:
    • Instead add a binding to a Vector<Op::Parameter>
    • and replace Op<Value<... TraceAccessBinding>> in the function/evaluator's body by Accessor<Vector<Op::Parameter>, TraceAccess> to the corresponding bound value.
  • Unpack during inlining

Mutability is not really handled through enums and conversions.

Mutating one occurence does not mutate others because conversion is done through cloning the underlying structure and wrapping that in a new Link.

  • Add Link<T> to enum variants' content, ie. :
enum Op {
    Add(Add),
    ...
}

becomes

enum Op {
    Add(Link<Add>)
    ...
}
  • Rework the builder pattern accordingly:
    • Builder works on Links directly, and mutate references instead of consuming self.
    • remove the last .build() step to allow storing partial but valid objects as their corresponding enum variant without adding new variants (their types are different)
    • remove unneeded .edit() abstraction.
  • Change parent from BackLink<Op> to Vec<BackLink<Op>> to allow sharing of an operation node:
    • Enable automatic parent setting in the builder pattern when adding children. They are currently not set at all.

Rework passes

After the previous is done, we will be able to work on:

  • inlining pass which currently mostly doesnt mutate anything if at all properly. It has been excluded temporarily from the pipeline while we work on fixing it.
  • unrolling pass which is not currently in the pipeline as it relies on the inlining pass.

cleanup the codebase

  • add a few tests for identified edegecases
  • convert relevant tests to compare parsed Mir between the input program and the expected optimized version.
  • document all public interfaces
  • refactor the boilerplate:
    • extract the builder pattern into a derive macro Builder
    • extract converters into As* and TryAs* traits (AsNode, AsOp, AsRoot, AsAdd, ..., TryAsNode) and add generic implementations where possible.

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