-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add INLINE to define inline functions and methods #1029
Conversation
I haven't reviewed but I just wanted to leave a few pre-comments.
Excited to see this. Inlining will be such a huge performance lever in Coalton that could transform it from being "inadequate" to being "faster than Lisp by default". |
Thank you for the pre-review! I'm still thinking what the right thing to do would be. But yes, I definitely want to see inlining in coalton. One main concern (EDIT: Fixed below using a node-let) I have with this approach is that it depends on the implementation to perform the inlining - see the following changes in src/codegen/program.lisp : (loop :for (name . node) :in bindings
:if (node-abstraction-p node)
:append (list
(if (node-abstraction-inline-p node)
`(declaim (inline ,name))
`(declaim (notinline ,name)))
(compile-function name node env)
`(setf
,name This means that suppose As I understand, avoiding this would require some more work, so that inlining can be performed within coalton compilation without depending on the implementation. |
We should definitely attempt to inline ourselves (at the AST level) so that further type and monomorphizer/specializer optimizations. Maybe in specific last-mile optimizations we would use the Lisp inliner. (I'm happy Lisp has inlining, but it's a very sharp and specific tool.) |
Hey, thanks for working on this. Wether a function is marked inline should be tracked in the I would prefer to use attributes instead of adding a new toplevel form:
The inliner should be written as a separate optimization pass instead of being merged with the Lastly, the inliner should generate fresh variable names instead of reusing the names of the functions's parameter names. This way we avoid issues with overlapping variable name. You can generate variable names with |
Thanks for the extensive feedback! I will rework this over the next week or two and push the commits for further review. Yes, now, I'm in favour of attributes over a new toplevel form. If a user wants, they can write a macro which expands into the attribute+form: (defmacro define-inline (name &body body)
`(progn
(inline)
(define ,name ,@body))) However, in the longer run, if we wish to provide more granular SBCL-like control over inlining, we might need something akin to declarations. (declaim (inline foo))
(defun foo (a)
(+ a a))
(declaim (notinline foo))
(defun foo-inline-caller (b)
(declare (inline foo))
(foo b))
(defun foo-mixed-caller (b)
(declare (inline foo))
(+ (foo b)
(locally (declare (notinline foo)) ; <------ Are coalton attributes suitable for this?
(foo b)))) |
It looks like creating a Without that, it seems we will need to change Let me know if I should proceed by adding a |
In every place (define-env-updater set-function-arity (env symbol arity)
(declare (type environment env)
(type symbol symbol)
(type fixnum arity))
(let ((prev (lookup-function env symbol :no-error t)))
(update-environment
env
:function-environment (immutable-map-set
(environment-function-environment env)
symbol
(make-function-env-entry
:name symbol
:arity arity
:inline-p (and prev (function-env-entry-inline-p prev)))
#'make-function-environment)))) |
Sorry, I've been busy. It's been great to see lots of activity on coalton recently! I might be free to dive into this in detail in another month. Is there any suggestion to break the changes required for this into multiple parts? That way, each part will be easier to review and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Don't worry about breaking this up, it's pretty reviewable.
Sorry I dropped the ball on reviewing this.
This needs some tests, but other than that and a couple nits this looks good to merge.
src/codegen/optimizer.lisp
Outdated
(node-type node)) | ||
:value name) | ||
:rands rands))))))) | ||
;; FIXME: What kind of frontend code evokes DIRECT-APPLICATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pass turns regular calls into direct calls when it's known that regular function calls can be used.
src/codegen/optimizer.lisp
Outdated
(when (node-variable-p rator) | ||
(let* ((name (node-variable-value rator)) | ||
(code (tc:lookup-code env name :no-error t)) | ||
;; FIXME: We need to lookup inline-p in the environment rather than the AST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this FIXME is done
src/typechecker/define-class.lisp
Outdated
@@ -358,7 +360,9 @@ | |||
:if (not (zerop method-arity)) | |||
:do (setf env (tc:set-function env method-name (tc:make-function-env-entry | |||
:name method-name | |||
:arity method-arity))) | |||
:arity method-arity | |||
;; FIXME: Always declare methods to be inline? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do this
src/typechecker/define-class.lisp
Outdated
@@ -334,7 +334,9 @@ | |||
:if (not (zerop class-arity)) | |||
:do (setf env (tc:set-function env codegen-sym (tc:make-function-env-entry | |||
:name codegen-sym | |||
:arity class-arity))) | |||
:arity class-arity | |||
;; FIXME: Under what circumstances would we want fundep to be inline? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class constructor functions can't be inlined by the Coalton inliner because they don't appear in the code env.
Hi, great PR! I wanted to jump in with some thoughts about laying the groundwork to implement heuristic inlining as mentioned by @stylewarning. In addition to this
This could be implemented by mostly copying the existing code for the monomorphizer, tweaking |
@kartik-s I think these are great ideas, but I think for this PR, we should rename @digikar99 Would it be possible to get this MR rebased, if it's not too much trouble? |
Sorry, I missed the last review by eliaslfox! I will be putting this on priority for the upcoming week or two. I think heuristic inlining and a monomorphize-like inline should rather be in a separate PR. Even implementing basic inline-ing seems to require a fair amount of changes. The currently missing "feature" for basic inlining is method inlining, that will allow inlining the (coalton-toplevel
(define-class (bar :a)
(add (:a -> :a -> :a))))
(coalton-toplevel
(define-instance (bar single-float)
(inline) ; <--- THIS ATTRIBUTE
(define (add x y)
(lisp single-float (x) (cl:+ x x)))))
(coalton-toplevel
(declare add-caller (single-float -> single-float -> single-float))
(define (add-caller x y)
(add x y))) Earlier I had mistakenly made changes in src/typechecker/define-class.lisp to achieve the same. But it seems the appropriate place for changes might be src/typechecker/define-instance.lisp. May be no changes are required to define-instance.lisp either, and instead, setting the INLINE-P-TABLE in ENTRY-POINT would do the job. I particularly need a review or suggestion on the ENTRY-POINT in src/entry.lisp. I've added an INLINE-P-TABLE which is populated from the contents of the parsed program. The INLINE-P slot in FUNCTION-ENV-ENTRY would be directly filled from INLINE-P-TABLE. This approach works for normal functions defined using toplevel DEFINE. However, this approach runs into an issue while dealing with instance-methods. The inline attribute is present in the parsed object, but the parsed object does not contain the codegen-sym that should be marked as inline in INLINE-P-TABLE. The TY-INSTANCES and TOPLEVEL-INSTANCES contain codegen-sym, but they do not contain information about INLINE-P - or atleast my effort would be to avoid polluting them with the attributes. My current issue is: what is a good way to go from method objects of (parser:program-instances program) to the codegen-syms corresponding to those methods? Or, is creating and filling the INLINE-P-TABLE in ENTRY-POINT a bad idea? |
Basic method inlining is now in place too! That leaves
Once those are in place, we can move the focus to "monomorphize-like" inline everything according to a heuristic, may be in a different PR. |
(in-package #:coalton-impl/codegen/constant-propagation) | ||
|
||
(defun constant-var-value (var constant-bindings) | ||
(cdr (assoc var constant-bindings))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when checking that value exists later you need the whole cell, not just its cdr
, which might still exist but be nil.
you can do this similar to gethash
:
(cdr (assoc var constant-bindings))) | |
(let ((x (assoc var constant-bindings))) | |
(if (null x) | |
(values nil nil) ; not found | |
(values x t)))) |
(and then change usage of this function accordingly)
(edit: actually, if the cdr
is always a node
(if it exists) then I guess this would be unnecessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if only there were a type system (:
I think this is ready for further review now - I need opinions and suggestions to resolve the FIXMEs and the remaining tasks mentioned in the top comment. |
The main inlining pass is now merged in #1193, so for this PR, now it suffices to just keep the changes for handling syntax Also, with the new |
Oops, I again missed an update here for quite a while! I have decreased my repository watch-level to a more appropriate level now! Given the state of this PR and the updates that have taken place in coalton to enable inlining, I think I will issue two separate PRs for
|
Great ideas! |
Closed in favor of the series of PRs that followed. |
This PR is a sketch to add inlinable functions and methods to coalton. List of tasks to complete before merge:
inline-p
okay, or should they be renamed toinline
? Although, I hope it doesn't create confusion withset-method-inline
and friends.The intended use is as follows:
Without
foo
beinginline
, the disassembly offoo-caller
would have included a call tofoo
as follows:But now that
foo
can beinline
d, the disassembly offoo-caller
is:This not only extends to regular functions, but also to class method instances.
Effectively, this brings us inlinable static dispatch with zero runtime overhead for calling simple functions.
In an earlier PR, it was suggested that aI am adding it as an attribute for the moment instead of any separate toplevel form. If someone has a preference for a toplevel form, they can always write a macro. The case for converting it into a CL-like declaration for more granular control remains.coalton:inline
declaration may be added to define inline functions and methods. I felt a slight inclination towards acoalton:define-inline
form instead of acoalton:inline
declaration. Nonetheless, if a declaration seems better, I will attempt to modify the PR accordingly.This is still a sketch since I haven't looked over every case carefully. I'd be also be delighted if this facility could be achieved by more minimal changes.