Re: [Chicken-hackers] [PATCH] fix #1648 (correctly)

[megane]

Hi,

I took a look at this. Four points:

1.

It fixes the runaway inlining. \o/

2.

I don't think the -unroll-limit is that useful option to expose for the
user. The -inline-limit already controls the amount of inlining. I
couldn't get anything to unroll more than once without having to
increase the inline-limit, which of course has other implications.

(Here I digress..

This made me wonder why didn't -inline-limit stop the unrolling in the
first place? I think the reason is that after the lambda is inlined with
inline-lambda-bindings the size of the lambda is not updated.

Maybe this could cause too big inlinings if the inlines are nesting?
)

address@hidden writes:

> From 5f0aec415b782023f9827b8ce14e499b148a335a Mon Sep 17 00:00:00 2001
> From: felix <address@hidden>
> Date: Wed, 18 Sep 2019 14:36:55 +0200
> Subject: [PATCH] Catch runaway inlining
>
[...]
>
>
> +;; Check whether inlined procedure has already been inlined in the
> +;; same target procedure and count occurrences. If the number of
> +;; inlinings exceed the unroll-limit

3.

The documentation cuts off abruptly here.

> +
> +(define (within-unrolling-limit fid tfid max-unrolls)
> +  (let ((p (cons fid tfid)))
> +    (let loop ((h inline-history) (n 0))
> +      (cond ((null? h))
> +            ((equal? p (car h))
> +             (and (< n max-unrolls)
> +                  (loop (cdr h) (add1 n))))
> +            (else (loop (cdr h) n))))))
> +

4.

This is unnecessarily O(n^2) in the number of performed inlines. I think
we should use a hash-table for inline-history.

I tested with some generated files and the results were:

Case n=100: real        0m1.784s  vs. real      0m2.179s  (1.2x)
Case n=200: real        0m7.495s  vs. real      0m12.235s (1.6x)
Case n=300: real        0m17.738s vs. real      0m42.660s (2.4x)
Case n=400: real        0m34.975s vs. real      2m3.604s  (3.5x)