robert-strandh / SICL

A fresh implementation of Common Lisp
Other
1.07k stars 79 forks source link

Fix closure conversion and make CST-to-AST do what Generate-AST does. #114

Closed karlosz closed 5 years ago

karlosz commented 6 years ago

Speeds up partial inlining noticeably.

Bike commented 6 years ago

Clasp built fine using this.

Bike commented 6 years ago

Using this to build Clasp, I got what looked like an miscompiled function using uninitialized data- it'll take me a while to find the culprit.

Bike commented 6 years ago

Rather than recomputing the binding site after the fact with dominance, I think it might be easier to just have CST-to-AST mark LET assignments specially (like a subclass of setq-ast, or whatnot).

karlosz commented 6 years ago

Not only would LET assignments need to be marked, but also variables bound by LAMBDA in the syntax tree. Since there may be an arbitrary number of optimizations made between AST->HIR and closure conversion we should consider the pros and cons of each approach: Using the dominance tree: Pros:

Cons:

Creating new AST nodes/marking the syntax tree. Pros:

Cons:

So it looks like the decision mainly comes down to whether there will be any serious optimizations worth doing besides inlining before closure conversion.

karlosz commented 6 years ago

http://ix.io/1jQi

Clasp backtrace posted by Bike.

karlosz commented 6 years ago

Generalized for no dominating definer

karlosz commented 6 years ago

This PR also makes generate AST closure convert properly

karlosz commented 6 years ago

Rebased with incremental inlining changes implemented by Bike.