Add INLINE syntax

[digikar99]

The final PR towards adding inlining support in coalton. The earlier ones include:

• Implement inlining with basic syntactic heuristics #1193
• Add a constant-propagation pass #1212

This PR adds support for (inline) syntax in (i) coalton toplevel (ii) inside define-instance before methods. See the file tests/inliner-tests.lisp for a number of examples.

Particular aspects I can think of reviewing in this PR include:

• if someone prefers the name inline to inline-p at some places
• adding more tests / improving them

But yes, let's address any other worries or issues others spot!

[macrologist]

Mostly nits

[macrologist]

when not needed here. If fun-env-entry is nil, the the whole when-let* form will be nil and the form bound to inline-p will never be evaluated.

[macrologist]

Similarly, inline-p will always be non-nil at this point if this code is evaluated at all.

[digikar99]

Would it make sense to rewrite this condition as the following?

                  (fun-env-entry (tc:lookup-function env name :no-error t))
                  (_    (and (node-abstraction-p code)
                             (or (tc:function-env-entry-inline-p fun-env-entry)
                                 (funcall heuristic code))
                             (<= (length call-stack)
                                 max-depth)

[amorphedstar]

Yeah, I think that is a good way to rewrite it so that it will work with the when-let*.
The only thing I'm unsure of is if there ever can be a case when there is an entry in the code-environment but not in the function-environment. (I can't tell easily just from looking at the code, since the code entries and function entries are added at very different places in the code.) If so, then in order to allow heuristic inlining of functions with code env entries but not function env entries, it seems like a separate let statement might be necessary, like

                  (_    (and (node-abstraction-p code)
                             (or (alexandria:when-let (fun-env-entry (tc:lookup-function env name :no-error t))
                                   (tc:function-env-entry-inline-p fun-env-entry))
                                 (funcall heuristic code))
                             (<= (length call-stack)
                                 max-depth)

or if not, then your suggestion should work exactly as written, or even without the :no-error t.

[digikar99]

If I remove :no-error t in my suggestion, I obtain the error "Internal coalton bug: Unknown function TREE:INCREASING-ORDER" while compiling library/ord-map.lisp. So, there indeed seem to be cases with code env entries but not function env entries.

However, I note that your suggestion works even without the :no-error t. My hypothesis is there are fun-env-entries only when code is an abstraction node.

[digikar99]

In fact, in those cases, code is an application node.

                  (fun-env-entry (let ((fun-env-entry (tc:lookup-function env name :no-error t)))
                                   (unless fun-env-entry (print (type-of code)))
                                   fun-env-entry))
                  (_    (and (node-abstraction-p code)
                             (or (tc:function-env-entry-inline-p fun-env-entry)
                                 (funcall heuristic code))

With the above code, I obtain COALTON-IMPL/CODEGEN/AST:NODE-APPLICATION as print statements while compiling library/ord-map.lisp and library/seq.lisp.

I'm inclined to commit your suggestion without :no-error t to better understand internal behavior. :no-error t feels like a hack here and I think it would rather be used as such.

[digikar99]

Committed in 849af1f

[macrologist]

nit: superfluous newline

[digikar99]

Is there a preference for the newlines under the (coalton:declare) clause below? Should I remove them?

[macrologist]

:for inline-p := (and (should-inline-p forms) 
                      (prog1 (parse-inline (cst:first forms) source)
                         (setf forms (cst:rest forms))))
:for method := (parse-instance-method-definition (cst:first forms) (cst:second form) source)
:when inline-p :do (setf (instance-method-definition-inline-p method) inline-p)
:collect :method 
:do (setf forms (cst:rest forms))

nit: this seems easier to follow to me. managing the state of forms is a little confusing to follow, I think something like the above makes it clearer.

In something like the above, error signalling would be moved into parse-inline. The should-inline-p wouldn't need to exist, I just hate writing code in github comment boxes so I replaced the check with that.

[digikar99]

nit: this seems easier to follow to me. managing the state of forms is a little confusing to follow, I think something like the above makes it clearer.

Unless I'm missing something, the suggested code still manually manipulates the state of forms. But I understand, this part of the code is a bit complicated to follow.

[macrologist]

that's correct. It's just a nit. Changes are not necessary

[shirok]

This is a general comment, probably for future work. As the optimizer gets more sophisticated, there'll be more context like monomorphize-table and inline-p-table to be passed. At some point we might want to consolidate them into optimizer context, maybe?

[amorphedstar]

I don't know what style guidelines we have for multi-line strings, but removing some indents here might make it nicer to read:

Suggested change

	"(inline)
	(define (factorial-1 n)
	(if (== n 0)
	1
	(* n (factorial-2 (- n 1)))))
	(inline)
	(define (factorial-2 n)
	(if (== n 0)
	1
	(* n (factorial-1 (- n 1)))))"))
	"(inline)
	(define (factorial-1 n)
	(if (== n 0)
	1
	(* n (factorial-2 (- n 1)))))
	(inline)
	(define (factorial-2 n)
	(if (== n 0)
	1
	(* n (factorial-1 (- n 1)))))"))

[amorphedstar]

You could add another test for duplicate inline attributes to match the tests for monomorphize ( https://github.com/search?q=repo%3Acoalton-lang%2Fcoalton+monomorphize+path%3A%2F%5Etests%5C%2Ftest-files%5C%2F%2F&type=code ) but other than that I think this looks pretty good and complete