Rewrite to include Decl in the standard AST
#951
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements #826 by adding a
TmDeclterm toExpr. Most of the changes are fairly direct translations to the new style (e.g.,TmLet {...}toTmDecl {decl = DeclLet {...}, ...}), but I have also made some refactorings making use of the new structure to remove code and make things simpler.As mentioned in the issue, the idea is to use
TmDeclfor everything where we have a "side-effect" followed by a single expression. For example,TmMatchdoes not fit this criteria (two options on how to continue), butTmLet(name-binding as a side-effect) andTmUtest(running a test) do.ast-builder.mcnow makesDecls when using, e.g.,let_. The separatemlang/ast-builder.mcno longer has, e.g.,decl_let_. Changed types:bind_ : Expr -> Expr -> Exprbecomesbind_ : Decl -> Expr -> Exprand can no longer implicitly lose AST nodesbindall_ : [Expr] -> Exprbecomesbindall_ : [Decl] -> Expr -> Expr.smap_Expr_ExprforTmDeclusesmap_Decl_Exprto make it behave as it used to, to minimize the number of hard to find semantic mismatches. We might want to change this later, or we might not, I'm not sure at this point.I also accidentally snuck in an additional feature for
generate-pprint.mc, the ability to register custom pprint functions for named (non-alias) types, similar to what's already available forgenerate-json-serializers.mc.Performance
This change increases the number of nodes in most ASTs, thus we expect there to be some impact on performance, both for runtime and memory. The runtime can perhaps be somewhat mitigated by simpler code that needs fewer checks, but the added pointer chasing and potential for cache misses and garbage collection seem likely to be greater.
Here are summary statistics over 10 runs for each step in the bootstrapping process, first before this PR, then after.
Step 1:
Step 2:
Step 3:
Data underlying the above
...and here's after:
In summary, we do indeed see a slowdown, most notably in the first step and then decreasing for later steps. The last step in particular is a slowdown of 0.09s on average, out of ~40s total, which seems quite acceptable.