Skip to content
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

Some evaluations return Int instead of Bool due to optimizer's work #5715

Closed
p6rt opened this issue Oct 1, 2016 · 9 comments
Closed

Some evaluations return Int instead of Bool due to optimizer's work #5715

p6rt opened this issue Oct 1, 2016 · 9 comments
Labels

Comments

@p6rt
Copy link

@p6rt p6rt commented Oct 1, 2016

Migrated from rt.perl.org#129782 (status was 'resolved')

Searchable as RT129782$

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 1, 2016

From @usev6

We have different tests in roast which are currently failing on JVM
because an Int is returned where a Bool is expected. In all cases the
problem goes away with --optimize=off. I believe all those tests fail
for the same reason.
 
This might be related to https://rt-archive.perl.org/perl6/Ticket/Display.html?id=127951 
where changing --optimize=2 to --optimize=1 makes the difference.
 
==== examples taken from roast
 
$ ./perl6-j -e 'say (1/10 + 1/10 + 1/10 == 0.3).gist' ## integration/advent2013-day15.t
1
$ ./perl6-j --optimize=off -e 'say (1/10 + 1/10 + 1/10 == 0.3).gist'
True
 
$ ./perl6-j -e 'my Mu $x = "a" ne ("a"|"b"|"c"); say $x ~~ Bool' ## S03-junctions/autothreading.t
False
$ ./perl6-j --optimize=off -e 'my Mu $x = "a" ne ("a"|"b"|"c"); say $x ~~ Bool'
True

$ ./perl6-j -e 'say (0 == 0).HOW' ## S03-operators/basic-types.t
Perl6​::Metamodel​::ClassHOW.new
$ ./perl6-j --optimize=off -e 'say (0 == 0).HOW'
Perl6​::Metamodel​::EnumHOW.new
 
$ ./perl6-j -e 'say (?^5); say (?^5).^name' ## S03-operators/boolean-bitwise.t
False
Int
$ ./perl6-j --optimize=off -e 'say (?^5); say (?^5).^name'
False
Bool
 
$ ./perl6-j -e 'say defined("a" => 5) ~~ Bool' ## S32-scalar/defined.t
False
$ ./perl6-j --optimize=off -e 'say defined("a" => 5) ~~ Bool'
True

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 2, 2016

From @usev6

Here are my preliminary findings about this problem.
I used the following evaluation for my debugging​:

$ ./perl6-j -e 'say (so 1).perl'
1

The optimizer generates a QAST​::Want with two children
(output generated with RAKUDO_OPTIMIZER_DEBUG=1)​:

[...]
- QAST​::Op(callstatic &say) <sunk> :statement_id<?> say (so 1).perl
  - QAST​::Op(hllize) <wanted>
  - QAST​::Op(callmethod perl) <> perl
  - QAST​::Want <>
  - QAST​::WVal(Bool) <>
  - Ii
  - QAST​::IVal(1) <>

I tried the following patch (skipping the creation of that
QAST​::Want if $ret_value is an instance of a Bool) and all
the tests mentioned in my original post passed afterward​:

====

Inline Patch
diff --git a/src/Perl6/Optimizer.nqp b/src/Perl6/Optimizer.nqp
index 12398ba..dcdd55b 100644
--- a/src/Perl6/Optimizer.nqp
+++ b/src/Perl6/Optimizer.nqp
@@ -1435,7 +1435,7 @@ class Perl6::Optimizer {
                         my $want;
                         if !nqp::isconcrete($ret_value) {
 # can we create a Want with a type object???  XXX
-                        } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Int")) && !nqp::isbig_I(nqp::decont($ret_value)) {
+                        } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Int")) && !nqp::istype($ret_value, $!symbols.find_in_setting("Bool")) && !nqp::isbig_I(nqp::decont($ret_value)) {
                             $want := QAST::Want.new($wval,
                                 "Ii", QAST::IVal.new(:value(nqp::unbox_i($ret_value))));
                         } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Num")) {
====

With that patch the evaluation gave​:

$ ./perl6-j -e 'say (so 1).perl'
Bool​::True

The generated QAST from the optimizer didn't had the Want
anymore (as expected)​:

[...]
- QAST​::Op(callstatic &say) <sunk> :statement_id<?> say (so 1).perl
  - QAST​::Op(hllize) <wanted>
  - QAST​::Op(callmethod perl) <> perl
  - QAST​::WVal(Bool) <>

In src/Perl6/Optimizer.nqp there is an comment that made me think
that those QAST​::Want were only meant for instances of Int, Num
or Str​: https://github.com/rakudo/rakudo/blob/nom/src/Perl6/Optimizer.nqp#L1433-L1434

We also generate those QAST​::Want for Bool or Mixins like here​:

$ ./perl6-j -e 'my $value = 42 but False; say ?$value'
True

[...]
- QAST​::Want <>
  - QAST​::WVal(Int+{<anon|1285927979>}) <>
  - Ii
  - QAST​::IVal(42) <>

I asked about my patch on #perl6-dev. There was no clear result,
but the tenor of the discussion was, that the JVM backend should
be able to interprete those QAST​::Want correctly.

==== start of discussion on IRC -- cmp. http://irclog.perlgeek.de/perl6-dev/2016-10-02#i_13323463
bartolin could someone take a look at usev6/rakudo@a547db7 ? It's an attempt to fix RT #​129782 but I'm not sure if there is indeed a (kind of) bug in the optimizer or if the fix has to happen in the JVM specific code.
synopsebot6 Link​: https://rt-archive.perl.org/perl6//Public/Bug/Display.html?id=129782
MasterDuke bartolin​: purely nitpicking, but if you put the added conditional at the end, the diff would be a little easier to read
MasterDuke (not that one line is all that difficult...)
bartolin MasterDuke​: thanks, noted. I wondered where the additional check should be placed wrt performance, since the order could make a difference performance wise, couldn't it? But, I've no idea how hot that path is
MasterDuke yeah, i was thinking about that after i said it
MasterDuke since i would definitely prioritize almost any performance difference over the minor change in readability of a one-line commit
* bartolin nods
psch r​: sub f(--> Bool) { so 1 }; say f().WHAT
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«(Bool)␤»
psch r​: sub f() { so 1 }; say f().WHAT
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«(Bool)␤»
psch r​: sub f() { so 1 }; say f().perl
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«Bool​::True␤»
psch r​: say (so 1).perl
camelia rakudo-jvm 2a1605​: OUTPUT«1␤»
camelia ..rakudo-moar 4abc28​: OUTPUT«Bool​::True␤»
psch that is honestly weird
psch bartolin​: i'm inclined to think that the QAST​::Want behavior is the actual problem though
psch r​: sub f { sub { say 1 } }; f()() # i am somehow reminded of this
camelia rakudo-moar 4abc28​: OUTPUT«1␤»
camelia ..rakudo-jvm 2a1605​: ( no output )
psch aw don't be shy camelia, show the NPE :P
psch r​: sub f { sub { say 1 } }; f()() # i
camelia rakudo-moar 4abc28​: OUTPUT«1␤»
camelia ..rakudo-jvm 2a1605​: OUTPUT«java.lang.NullPointerException␤ in block <unit> at <tmp> line 1␤␤»
bartolin psch​: so, you would say it makes sense to have a QAST​::Want with a QAST​::WVal for the 'True' and a QAST​::IVal for the '1'?
bartolin psch​: ... and the interpretation of that QAST​::Want on JVM is wrong?
psch bartolin​: well, moar inlines it to Want <> with WVal(Bool) and IVal(1) as children
psch bartolin​: the &so call, that is
bartolin psch​: oh, your last evaluation does not give a NPE with --optimize=off -- I didn't know that
bartolin psch​: yeah, the QAST is the same on JVM (if I'm not mistaken)
psch bartolin​: and considering the post-optimize QAST looks the same across the backends but doesn't give the same result... well there's probably something wrong in the wrong backend :)
psch bartolin​: i'd guess it's probably something with how we have to do returns out of band on jvm and sometimes don't carry the right $*WANT around or so
psch bartolin​: but honestly those bits of the QAST -> JAST Compiler aren't particularly transparent to me :)
bartolin psch​: from the comments (https://github.com/rakudo/rakudo/blob/nom/src/Perl6/Optimizer.nqp#L1433) I got the impression that those QAST​::Want are really only meant for Int, Num and Str (in order to add a native variant) -- and not for Bool
bartolin timotimo​: maybe you can shed some light? (wrt usev6/rakudo@a547db7)
timotimo whoops, that want is totally broken :)
timotimo it should have the Ii and the IVal at the same level as the WVal(Bool)
bartolin timotimo​: sorry, that was my fault
timotimo oh, phew
bartolin fixed
timotimo your patch may be right
bartolin r​: my $value = 42 but False; say ?$value
camelia rakudo-jvm 2a1605​: OUTPUT«True␤»
camelia ..rakudo-moar 28c23a​: OUTPUT«False␤»
timotimo o_O
bartolin r-j gives False with --optimize=off here
timotimo ugh ;(
bartolin I guess it's the same problem (according to the output generated with RAKUDO_OPTIMIZER_DEBUG=1)
timotimo might be, yeah :\
bartolin so, if my patch makes sense, it is not complete
psch bartolin​: well, Bool is Int on one hand
psch bartolin​: and on the other, there's demonstrably other cases where the optimizer produces something related to Wants that works on moar and doesn't on jvm
psch bartolin​: so i think the optimizer isn't to blame
bartolin psch​: yeah. but do we really need the Want there? I mean, both the optimizer and the JVM backend could be wrong :-)
psch m​: sub f(int $) { say "yup" }; f Bool
camelia rakudo-moar 28c23a​: OUTPUT«Cannot unbox a type object␤ in sub f at <tmp> line 1␤ in block <unit> at <tmp> line 1␤␤»
psch m​: sub f(int $) { say "yup" }; f True
camelia rakudo-moar 28c23a​: OUTPUT«yup␤»
psch j​: sub f(int $) { say "yup" }; f True
camelia rakudo-jvm 2a1605​: OUTPUT«yup␤»
psch bartolin​: afaiu the Want means "here's the native value in case you need it", and an IVal makes perfect sense for in the case of Bool
bartolin psch​: ok, at least I understood the meaning of the Want, then :-)
timotimo well, something is mishandling that case, though
psch yeah, the Want handling in JAST​::Compiler, probably :S
psch but that seems to be complicated stuff and i don't get it
timotimo right, the jast compiler isn't easy
bartolin ok, so I won't open a PR then, but will add the discussion to the ticket
==== end of discussion on IRC -- powered by https://github.com/usev6/dump-irc-logs

1 similar comment
@p6rt
Copy link
Author

@p6rt p6rt commented Oct 2, 2016

From @usev6

Here are my preliminary findings about this problem.
I used the following evaluation for my debugging​:

$ ./perl6-j -e 'say (so 1).perl'
1

The optimizer generates a QAST​::Want with two children
(output generated with RAKUDO_OPTIMIZER_DEBUG=1)​:

[...]
- QAST​::Op(callstatic &say) <sunk> :statement_id<?> say (so 1).perl
  - QAST​::Op(hllize) <wanted>
  - QAST​::Op(callmethod perl) <> perl
  - QAST​::Want <>
  - QAST​::WVal(Bool) <>
  - Ii
  - QAST​::IVal(1) <>

I tried the following patch (skipping the creation of that
QAST​::Want if $ret_value is an instance of a Bool) and all
the tests mentioned in my original post passed afterward​:

====

Inline Patch
diff --git a/src/Perl6/Optimizer.nqp b/src/Perl6/Optimizer.nqp
index 12398ba..dcdd55b 100644
--- a/src/Perl6/Optimizer.nqp
+++ b/src/Perl6/Optimizer.nqp
@@ -1435,7 +1435,7 @@ class Perl6::Optimizer {
                         my $want;
                         if !nqp::isconcrete($ret_value) {
 # can we create a Want with a type object???  XXX
-                        } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Int")) && !nqp::isbig_I(nqp::decont($ret_value)) {
+                        } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Int")) && !nqp::istype($ret_value, $!symbols.find_in_setting("Bool")) && !nqp::isbig_I(nqp::decont($ret_value)) {
                             $want := QAST::Want.new($wval,
                                 "Ii", QAST::IVal.new(:value(nqp::unbox_i($ret_value))));
                         } elsif nqp::istype($ret_value, $!symbols.find_in_setting("Num")) {
====

With that patch the evaluation gave​:

$ ./perl6-j -e 'say (so 1).perl'
Bool​::True

The generated QAST from the optimizer didn't had the Want
anymore (as expected)​:

[...]
- QAST​::Op(callstatic &say) <sunk> :statement_id<?> say (so 1).perl
  - QAST​::Op(hllize) <wanted>
  - QAST​::Op(callmethod perl) <> perl
  - QAST​::WVal(Bool) <>

In src/Perl6/Optimizer.nqp there is an comment that made me think
that those QAST​::Want were only meant for instances of Int, Num
or Str​: https://github.com/rakudo/rakudo/blob/nom/src/Perl6/Optimizer.nqp#L1433-L1434

We also generate those QAST​::Want for Bool or Mixins like here​:

$ ./perl6-j -e 'my $value = 42 but False; say ?$value'
True

[...]
- QAST​::Want <>
  - QAST​::WVal(Int+{<anon|1285927979>}) <>
  - Ii
  - QAST​::IVal(42) <>

I asked about my patch on #perl6-dev. There was no clear result,
but the tenor of the discussion was, that the JVM backend should
be able to interprete those QAST​::Want correctly.

==== start of discussion on IRC -- cmp. http://irclog.perlgeek.de/perl6-dev/2016-10-02#i_13323463
bartolin could someone take a look at usev6/rakudo@a547db7 ? It's an attempt to fix RT #​129782 but I'm not sure if there is indeed a (kind of) bug in the optimizer or if the fix has to happen in the JVM specific code.
synopsebot6 Link​: https://rt-archive.perl.org/perl6//Public/Bug/Display.html?id=129782
MasterDuke bartolin​: purely nitpicking, but if you put the added conditional at the end, the diff would be a little easier to read
MasterDuke (not that one line is all that difficult...)
bartolin MasterDuke​: thanks, noted. I wondered where the additional check should be placed wrt performance, since the order could make a difference performance wise, couldn't it? But, I've no idea how hot that path is
MasterDuke yeah, i was thinking about that after i said it
MasterDuke since i would definitely prioritize almost any performance difference over the minor change in readability of a one-line commit
* bartolin nods
psch r​: sub f(--> Bool) { so 1 }; say f().WHAT
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«(Bool)␤»
psch r​: sub f() { so 1 }; say f().WHAT
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«(Bool)␤»
psch r​: sub f() { so 1 }; say f().perl
camelia rakudo-jvm 2a1605, rakudo-moar 4abc28​: OUTPUT«Bool​::True␤»
psch r​: say (so 1).perl
camelia rakudo-jvm 2a1605​: OUTPUT«1␤»
camelia ..rakudo-moar 4abc28​: OUTPUT«Bool​::True␤»
psch that is honestly weird
psch bartolin​: i'm inclined to think that the QAST​::Want behavior is the actual problem though
psch r​: sub f { sub { say 1 } }; f()() # i am somehow reminded of this
camelia rakudo-moar 4abc28​: OUTPUT«1␤»
camelia ..rakudo-jvm 2a1605​: ( no output )
psch aw don't be shy camelia, show the NPE :P
psch r​: sub f { sub { say 1 } }; f()() # i
camelia rakudo-moar 4abc28​: OUTPUT«1␤»
camelia ..rakudo-jvm 2a1605​: OUTPUT«java.lang.NullPointerException␤ in block <unit> at <tmp> line 1␤␤»
bartolin psch​: so, you would say it makes sense to have a QAST​::Want with a QAST​::WVal for the 'True' and a QAST​::IVal for the '1'?
bartolin psch​: ... and the interpretation of that QAST​::Want on JVM is wrong?
psch bartolin​: well, moar inlines it to Want <> with WVal(Bool) and IVal(1) as children
psch bartolin​: the &so call, that is
bartolin psch​: oh, your last evaluation does not give a NPE with --optimize=off -- I didn't know that
bartolin psch​: yeah, the QAST is the same on JVM (if I'm not mistaken)
psch bartolin​: and considering the post-optimize QAST looks the same across the backends but doesn't give the same result... well there's probably something wrong in the wrong backend :)
psch bartolin​: i'd guess it's probably something with how we have to do returns out of band on jvm and sometimes don't carry the right $*WANT around or so
psch bartolin​: but honestly those bits of the QAST -> JAST Compiler aren't particularly transparent to me :)
bartolin psch​: from the comments (https://github.com/rakudo/rakudo/blob/nom/src/Perl6/Optimizer.nqp#L1433) I got the impression that those QAST​::Want are really only meant for Int, Num and Str (in order to add a native variant) -- and not for Bool
bartolin timotimo​: maybe you can shed some light? (wrt usev6/rakudo@a547db7)
timotimo whoops, that want is totally broken :)
timotimo it should have the Ii and the IVal at the same level as the WVal(Bool)
bartolin timotimo​: sorry, that was my fault
timotimo oh, phew
bartolin fixed
timotimo your patch may be right
bartolin r​: my $value = 42 but False; say ?$value
camelia rakudo-jvm 2a1605​: OUTPUT«True␤»
camelia ..rakudo-moar 28c23a​: OUTPUT«False␤»
timotimo o_O
bartolin r-j gives False with --optimize=off here
timotimo ugh ;(
bartolin I guess it's the same problem (according to the output generated with RAKUDO_OPTIMIZER_DEBUG=1)
timotimo might be, yeah :\
bartolin so, if my patch makes sense, it is not complete
psch bartolin​: well, Bool is Int on one hand
psch bartolin​: and on the other, there's demonstrably other cases where the optimizer produces something related to Wants that works on moar and doesn't on jvm
psch bartolin​: so i think the optimizer isn't to blame
bartolin psch​: yeah. but do we really need the Want there? I mean, both the optimizer and the JVM backend could be wrong :-)
psch m​: sub f(int $) { say "yup" }; f Bool
camelia rakudo-moar 28c23a​: OUTPUT«Cannot unbox a type object␤ in sub f at <tmp> line 1␤ in block <unit> at <tmp> line 1␤␤»
psch m​: sub f(int $) { say "yup" }; f True
camelia rakudo-moar 28c23a​: OUTPUT«yup␤»
psch j​: sub f(int $) { say "yup" }; f True
camelia rakudo-jvm 2a1605​: OUTPUT«yup␤»
psch bartolin​: afaiu the Want means "here's the native value in case you need it", and an IVal makes perfect sense for in the case of Bool
bartolin psch​: ok, at least I understood the meaning of the Want, then :-)
timotimo well, something is mishandling that case, though
psch yeah, the Want handling in JAST​::Compiler, probably :S
psch but that seems to be complicated stuff and i don't get it
timotimo right, the jast compiler isn't easy
bartolin ok, so I won't open a PR then, but will add the discussion to the ticket
==== end of discussion on IRC -- powered by https://github.com/usev6/dump-irc-logs

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 2, 2016

@usev6 - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 3, 2016

From @usev6

I think I found a clean way to fix the problem​: Raku/nqp#309

1 similar comment
@p6rt
Copy link
Author

@p6rt p6rt commented Oct 3, 2016

From @usev6

I think I found a clean way to fix the problem​: Raku/nqp#309

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 6, 2016

From @usev6

Fixed with Raku/nqp@5769a65 and Raku/nqp@2d88d98

I'm closing this ticket as 'resolved'.

1 similar comment
@p6rt
Copy link
Author

@p6rt p6rt commented Oct 6, 2016

From @usev6

Fixed with Raku/nqp@5769a65 and Raku/nqp@2d88d98

I'm closing this ticket as 'resolved'.

@p6rt
Copy link
Author

@p6rt p6rt commented Oct 6, 2016

@usev6 - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this Oct 6, 2016
@p6rt p6rt added the JVM label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant