Re: [tclquadcode] Defaults cause confusion on dkf-callframe-impl branch

[Kevin K. <kev...@gm...>]

On Sun, May 28, 2017 at 3:58 PM, Kevin Kenny <kev...@gm...>
wrote:

> On Sun, May 28, 2017 at 3:10 AM, Donal K. Fellows <
> don...@ma...> wrote:
>
>> In the case I'm reporting, the problem call is to [cos] from either one
>> of [coscaller1] or [coscaller2]. They have both decided that the [cos] they
>> wish to call is the [cos] that takes a single DOUBLE argument, and the
>> specializer has asked that this additional version be created, but the
>> quadcode for that version still internally asks for the second parameter.
>> LLVM (understandably!) chokes on being asked for the second parameter to a
>> one-argument function, so I need an alternative approach.
>>
>
> I'm starting to have a look at that now. I'm guessing that somehow we're
> committed to making a one-arg [cos] before the
> [varargs] pass has had a chance to supply the default. In that case, it
> would not surprise me if nobody calls it, because
> [varargs] ought to rewrite the call site. There's something broken in the
> plumbing, and this should be an easy fix.
>

There was indeed something badly broken in the plumbing. Said badly broken
thing should be fixed in [1facb9b83c]
<http://localhost:8080/info/1facb9b83c38da21a33abe11580e1d43bf6ef653>. The
'varargs' pass was not accounting for the extra callframe parameters in the
[invoke] quadcode. The only reason it worked at all is that a callframe is
not a procedure, so it never found a procedure definition to attempt to
supply default args. The fix handles both 'cos' (tests a default param) and
'xsum2' (tests 'args'), so I think it covers the cases. I also took the
opportunity to do a drive-by refactoring of that procedure, and added some
code so that '-quadcode-log varargs' on the options will cause it to log
what it's doing.

The fix still lacks code for the case where it is provable that a given
'invoke' will throw a 'wrong # args' error at run time. I can probably work
that out, by compiling a 'return -code error' and seeing what results. It's
something I need a more generic procedure for, since I've needed in several
other places the ability to throw an error unconditionally.
'throwNotExists' and 'throwArithDomainError' are quadcodes that wouldn't
need to exist if I knew how to emit such a beast.

After the fix is applied, the only other out-and-out failures that I see
are:

(1) Even with 'cleanopt' some tests are fussing about the error options
dictionary.

(2) 'mrtest::*' is hanging in the specialization phase for some reason. I
suspect that there's still some line of code lurking about that's failing
to account for the callframe arg on 'invoke'. I'll track this one down.

(3) A great many tests are slower than uncompiled, probably because it
takes us longer to construct and push a callframe than it does the Core.

(4) The 'kbk-callframe-motion' branch is failing on the 'regsub' test as
well, so I'm postponing merging it until I've tracked that one down.

Aside from (4), 'kbk-calframe-motion' seems to be buying back at least some
performance, by avoiding moveToCallframe and moveFromCallframe on
invocations of Core commands that don't touch the callframe. This saves not
only the immediate boxing and unboxing, but also avoids losing type
information on the unaffected variables, which is a significant win.

Once I've tracked down the 'mrtest' and 'regsub' failures, I need to get on
with callframe elimination, so that we can get back to our previous level
of performance. I had not realized how ludicrously expensive callframe
operations would prove to be!

I also hadn't realized what a large fraction of the tests pass in the
dkf-callframe-impl branch. That was a pleasant surprise, and now I'm a lot
less afraid to merge into it, since I know Donal doesn't have complicated
changes in a sandbox from that branch.

Crawling forward again...

Kevin