taillcall into PIR compiler broken

[coke]

See  Original RT

If you apply this patch to parrot:

index 776a062..4f1486f 100644
--- a/compilers/pct/src/PCT/HLLCompiler.pir
+++ b/compilers/pct/src/PCT/HLLCompiler.pir
@@ -512,8 +512,7 @@ Transform PAST C<source> into POST.
     .param pmc adverbs         :slurpy :named
 
     $P0 = compreg 'PIR'
-    $P1 = $P0(source)
-    .return ($P1)
+    .tailcall $P0(source)
 .end

This causes failures in rakudo. For example, "class {}" without the patch returns nothing in interactive mode, but with the patch, it returns a failure regarding the # of arguments expected.

A simple test of the tailcall (below) doesn't show this behavior.

.sub main 
  $P1 = elm()
  $P1()
.end

.sub elm

  $P1 = compreg 'PIR'
  $S0 =<<'END_PIR'
.sub foo
say 'ok'
.end
END_PIR
  .tailcall $P1($S0)
.end

[coke]

It would be interesting to find out if PIRC avoided this issue.

[coke]

Applying this patch to r44285 causes failures in "make nqp_test". No rakudo needed.

[coke]

per moritz++, current rakudo (as of today) fails to build with this patch.

[jkeenan]

Replying to coke:

Applying this patch to r44285 causes failures in "make nqp_test". No rakudo needed.

Where is make nqp_test found?

[parrot] 504 $ fns . | xargs grep -Ei 'nqp_?test'

[rakudo-star-2010.07] 505 $ grep -n nqp_test Makefile 
[rakudo-star-2010.07] 506 $ ack nqp_test *

What am I missing here?

[coke]

nqp_test is from the old nqp compiler, which rakudo used to be based on.

A simple 'make test' shows the failure is cropping up elsewhere now anyway:

prove t/profiling/profiling.tt

Passes without the patch, fails with it.

[coke]

That's prove t/profiling/profiling.t

[whiteknight]

short bit of analysis here. The tailcall happens without a problem. The PIR compreg resets interp->current_cont to NEEDS_CONTINUATION. The NCI VTABLE_invoke returns the old cur_opcode+2 destination instead of the updated destination from the interp->current_cont. The runloop continues execution past the end of the function, which leads to any number of potential problems. I'm testing a fix for this right now.

[whiteknight]

Fixed in 8b188db. All tests pass.

[coke]

Part of this change was reverted in 9328cf.

See #1935.

[pmichaud]

The tailcall has been reverted in 9328cff, as it was causing failures in both Rakudo and nqp-rx.

I'll add a note to this ticket that we probably should use the tailcall in HLLCompiler only to evoke a known Parrot bug, but not as a permanent "optimization" or change. In this particular case (and in most of HLLCompiler) there's no significant optimization to be had in using tailcalls, and there are definite negatives if the tailcalls are used in situations where they cause crashes and failures for downstream users.

Pm

[coke]

> Comment(by pmichaud):
>
>  The tailcall has been reverted in 9328cff, as it was causing failures in
>  both Rakudo and nqp-rx.
>
>  I'll add a note to this ticket that we probably should use the tailcall in
>  HLLCompiler only to evoke a known Parrot bug, but not as a permanent
>  "optimization" or change.  In this particular case (and in most of
>  HLLCompiler) there's no significant optimization to be had in using
>  tailcalls, and there are definite negatives if the tailcalls are used in
>  situations where they cause crashes and failures for downstream users.

FYI, the original bug report was against the old NQP, and was reduced
to a parrot test when it was noticed that it was failing just fine
with the patch.

It would be great if we could get a test that exhibits the current
failure as a core parrot test (I realize this is difficult.)

I think the goal should be to be fix the bug, not avoid it. (Though
avoiding it until it's fixed is, of course, a great idea.)

-- 
Will "Coke" Coleda

[whiteknight]

I have added a test for the behavior in ef82f5c, along with another more robust fix for the issue. We have removed the .tailcall from HLLCompiler and we do not need to replace it, but I think we are getting to a point where this issue is, or is nearly fixed. I am running some more tests now to verify.

[whiteknight]

Okay, i've verified that my newest fix resolves the issues that NQP was seeing. There is also a test now in place to verify that this same thing doesn't happen again, although I still don't know if .tailcall works 100%. It certainly is underutilized in the test suite and elsewhere.

I'm going to re-close this issue. Thanks to everybody who helped test this one.