From @jimc

---------- Forwarded message ----------
From​: Jim Cromie <jim.cromie@​gmail.com>
Date​: Mon, Aug 30, 2010 at 2​:30 AM
Subject​: Re​: [PATCH] Support B​::Generate and B​::C
To​: Reini Urban <rurban@​x-ray.at>, The Perl5 Porters Mailing List <
perl5-porters@​perl.org>

Yes, I realise now, that this was not as comprehensive as I perhaps could
have
been, as I didn't check all unindexed distributions.

You patch has no tests. For the documentation for fold_constants I see​:

attached is a patch adding fold-constant tests;
number arithmetic fold,
string catenation fold,
lc* & uc* folds
various string compare conditional folds
boolean constant conditional fold,
mixed str-cat . arithmetic fold, with () for forced precendence.
and TODO version without the ()

So does that mean that it's valid in future for this function to stick to
*that* documentation, and only fold constants?

As I discovered by accident last night, it also

b​: creates targets for ops that need targets but don't yet have them

and I see from looking at the code it also

c​: propagates scalar context for ops that return a scalar

d​: runs LINKLIST()

e​: implements 'use integer';

can you suggest perl statements that trigger these particular cases ?

As it's not documented as doing any of those, is it OK for a future version
of
perl to stop doing those? (Which might happen as a side effect of
refactoring)

Nicholas Clark

From @jimc

From f08de5533fad89b1364b4b53884c08ce219cf25f Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 29 Aug 2010 15:43:49 -0600
Subject: [PATCH 1/2] add constant expression folding tests

test addition, string catenation,
constant args to lc*, uc* and conditionals with gt, lt, ge, le, cmp
also 2 mixed string-cat . arithmetic constant tests,
1st with () to force precendence, 2nd TODO without ()
---
 ext/B/t/optree_constants.t |  178 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 177 insertions(+), 1 deletions(-)

diff --git a/ext/B/t/optree_constants.t b/ext/B/t/optree_constants.t
index 47afea4..4f1f727 100644
--- a/ext/B/t/optree_constants.t
+++ b/ext/B/t/optree_constants.t
@@ -13,7 +13,7 @@ BEGIN {
 use OptreeCheck;	# ALSO DOES @ARGV HANDLING !!!!!!
 use Config;
 
-my $tests = 30;
+my $tests = 36;
 plan tests => $tests;
 SKIP: {
 skip "no perlio in this build", $tests unless $Config::Config{useperlio};
@@ -243,6 +243,182 @@ checkOptree ( name	=> 'call many in a print statement',
 	      strip_open_hints => 1,
 	      expect => $expect, expect_nt => $expect_nt);
 
+# test constant expression folding
+
+checkOptree ( name	=> 'arithmetic constant folding in print',
+	      code	=> 'print 1+2+3',
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 937 (eval 53):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const[IV 6] s ->4
+EOT_EOT
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 937 (eval 53):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const(IV 6) s ->4
+EONT_EONT
+
+checkOptree ( name	=> 'string constant folding in print',
+	      code	=> 'print "foo"."bar"',
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 942 (eval 55):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const[PV "foobar"] s ->4
+EOT_EOT
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 942 (eval 55):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const(PV "foobar") s ->4
+EONT_EONT
+
+checkOptree ( name	=> 'boolean or folding',
+	      code	=> 'print "foobar" if 1 or 0',
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 942 (eval 55):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const[PV "foobar"] s ->4
+EOT_EOT
+# 5  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->5
+# 1        <;> nextstate(main 942 (eval 55):1) v ->2
+# 4        <@> print sK ->5
+# 2           <0> pushmark s ->3
+# 3           <$> const(PV "foobar") s ->4
+EONT_EONT
+
+checkOptree ( name	=> 'lc*,uc*,gt,lt,ge,le,cmp',
+	      code	=> sub {
+		  $s = uc('foo.').ucfirst('bar.').lc('LOW.').lcfirst('LOW');
+		  print "lt-a-b" if "a" lt "b";
+		  print "gt-b-a" if "b" gt "a";
+		  print "le-a-b" if "a" le "b";
+		  print "ge-b-a" if "b" ge "a";
+		  print "cmp-b-a" if "b" cmp "a";
+	      },
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+# p  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->p
+# 1        <;> nextstate(main 776 optree_constants.t:307) v:{ ->2
+# 4        <2> sassign vKS/2 ->5
+# 2           <$> const[PV "FOO.Bar.low.lOW"] s ->3
+# -           <1> ex-rv2sv sKRM*/1 ->4
+# 3              <$> gvsv[*s] s ->4
+# 5        <;> nextstate(main 776 optree_constants.t:308) v:{ ->6
+# 8        <@> print vK ->9
+# 6           <0> pushmark s ->7
+# 7           <$> const[PV "lt-a-b"] s ->8
+# 9        <;> nextstate(main 776 optree_constants.t:309) v:{ ->a
+# c        <@> print vK ->d
+# a           <0> pushmark s ->b
+# b           <$> const[PV "gt-b-a"] s ->c
+# d        <;> nextstate(main 776 optree_constants.t:310) v:{ ->e
+# g        <@> print vK ->h
+# e           <0> pushmark s ->f
+# f           <$> const[PV "le-a-b"] s ->g
+# h        <;> nextstate(main 776 optree_constants.t:311) v:{ ->i
+# k        <@> print vK ->l
+# i           <0> pushmark s ->j
+# j           <$> const[PV "ge-b-a"] s ->k
+# l        <;> nextstate(main 776 optree_constants.t:312) v:{ ->m
+# o        <@> print sK ->p
+# m           <0> pushmark s ->n
+# n           <$> const[PV "cmp-b-a"] s ->o
+EOT_EOT
+# p  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->p
+# 1        <;> nextstate(main 776 optree_constants.t:307) v:{ ->2
+# 4        <2> sassign vKS/2 ->5
+# 2           <$> const(PV "FOO.Bar.low.lOW") s ->3
+# -           <1> ex-rv2sv sKRM*/1 ->4
+# 3              <$> gvsv(*s) s ->4
+# 5        <;> nextstate(main 776 optree_constants.t:308) v:{ ->6
+# 8        <@> print vK ->9
+# 6           <0> pushmark s ->7
+# 7           <$> const(PV "lt-a-b") s ->8
+# 9        <;> nextstate(main 776 optree_constants.t:309) v:{ ->a
+# c        <@> print vK ->d
+# a           <0> pushmark s ->b
+# b           <$> const(PV "gt-b-a") s ->c
+# d        <;> nextstate(main 776 optree_constants.t:310) v:{ ->e
+# g        <@> print vK ->h
+# e           <0> pushmark s ->f
+# f           <$> const(PV "le-a-b") s ->g
+# h        <;> nextstate(main 776 optree_constants.t:311) v:{ ->i
+# k        <@> print vK ->l
+# i           <0> pushmark s ->j
+# j           <$> const(PV "ge-b-a") s ->k
+# l        <;> nextstate(main 776 optree_constants.t:312) v:{ ->m
+# o        <@> print sK ->p
+# m           <0> pushmark s ->n
+# n           <$> const(PV "cmp-b-a") s ->o
+EONT_EONT
+
+checkOptree ( name	=> 'mixed constant folding, with explicit precendence',
+	      code	=> '$i=1; print "foo"."bar".(2+3+$i)',
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+# d  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->d
+# 1        <;> nextstate(main 955 (eval 60):1) v:{ ->2
+# 4        <2> sassign vKS/2 ->5
+# 2           <$> const[IV 1] s ->3
+# -           <1> ex-rv2sv sKRM*/1 ->4
+# 3              <$> gvsv[*i] s ->4
+# 5        <;> nextstate(main 955 (eval 60):1) v:{ ->6
+# c        <@> print sK ->d
+# 6           <0> pushmark s ->7
+# b           <2> concat[t4] sK/2 ->c
+# 7              <$> const[PV "foobar"] s ->8
+# a              <2> add[t3] sKP/2 ->b
+# 8                 <$> const[IV 5] s ->9
+# -                 <1> ex-rv2sv sK/1 ->a
+# 9                    <$> gvsv[*i] s ->a
+EOT_EOT
+# d  <1> leavesub[1 ref] K/REFC,1 ->(end)
+# -     <@> lineseq KP ->d
+# 1        <;> nextstate(main 955 (eval 60):1) v:{ ->2
+# 4        <2> sassign vKS/2 ->5
+# 2           <$> const(IV 1) s ->3
+# -           <1> ex-rv2sv sKRM*/1 ->4
+# 3              <$> gvsv(*i) s ->4
+# 5        <;> nextstate(main 955 (eval 60):1) v:{ ->6
+# c        <@> print sK ->d
+# 6           <0> pushmark s ->7
+# b           <2> concat[t4] sK/2 ->c
+# 7              <$> const(PV "foobar") s ->8
+# a              <2> add[t3] sKP/2 ->b
+# 8                 <$> const(IV 5) s ->9
+# -                 <1> ex-rv2sv sK/1 ->a
+# 9                    <$> gvsv(*i) s ->a
+EONT_EONT
+
+checkOptree ( name	=> 'mixed constant folding',
+	      code	=> '$i=1; print "foo"."bar". 2+3+$i',
+	      todo	=> 1,
+	      strip_open_hints => 1,
+	      expect => <<'EOT_EOT', expect_nt => <<'EONT_EONT');
+string folding breaks when arithmetic folding is also done
+EOT_EOT
+string folding breaks when arithmetic folding is also done
+EONT_EONT
+
 } #skip
 
 __END__
-- 
1.7.2.2

From @jimc

From b6759ecfee39701442dff777bab4e2467ef6dc31 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 29 Aug 2010 16:40:34 -0600
Subject: [PATCH 2/2] add -Ilib -Iext/B to runperl calls

adding these to @INC lets tests find O.pm when run bare,
ie not inside make test.  Without it, some of these tests
fail, despite having the -Iargs passed in.

    for t in ext/B/t/*.t; do
	echo $t; ./perl -Ilib -I ext/B -I ext/B/t/ $t
    done
---
 ext/B/t/OptreeCheck.pm |    3 ++-
 ext/B/t/concise.t      |   20 +++++++++++++-------
 ext/B/t/showlex.t      |    2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/ext/B/t/OptreeCheck.pm b/ext/B/t/OptreeCheck.pm
index ce2482d..b0f3d53 100644
--- a/ext/B/t/OptreeCheck.pm
+++ b/ext/B/t/OptreeCheck.pm
@@ -511,7 +511,8 @@ sub getRendering {
 
 
     if ($tc->{prog}) {
-	$rendering = runperl( switches => ['-w',join(',',"-MO=Concise",@opts)],
+	$rendering = runperl( switches => ['-w', '-Ilib', '-Iext/B', 
+					   join(',',"-MO=Concise",@opts)],
 			      prog => $tc->{prog}, stderr => 1,
 			      ); # verbose => 1);
     } else {
diff --git a/ext/B/t/concise.t b/ext/B/t/concise.t
index 3eb22ce..eecf500 100644
--- a/ext/B/t/concise.t
+++ b/ext/B/t/concise.t
@@ -14,7 +14,7 @@ plan tests => 157;
 
 require_ok("B::Concise");
 
-$out = runperl(switches => ["-MO=Concise"], prog => '$a', stderr => 1);
+$out = runperl(switches => ["-Iext/B", "-MO=Concise"], prog => '$a', stderr => 1);
 
 # If either of the next two tests fail, it probably means you need to
 # fix the section labeled 'fragile kludge' in Concise.pm
@@ -366,14 +366,16 @@ SKIP: {
 # re-vivified later, but not in time for this (BEGIN/CHECK)-time
 # rendering.
 
-$out = runperl ( switches => ["-MO=Concise,Config::AUTOLOAD"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,Config::AUTOLOAD"],
 		 prog => 'use Config; BEGIN { $Config{awk} }',
 		 stderr => 1 );
 
 like($out, qr/Config::AUTOLOAD exists in stash, but has no START/,
     "coderef properly undefined");
 
-$out = runperl ( switches => ["-MO=Concise,Config::AUTOLOAD"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,Config::AUTOLOAD"],
 		 prog => 'use Config; CHECK { $Config{awk} }',
 		 stderr => 1 );
 
@@ -383,7 +385,8 @@ like($out, qr/Config::AUTOLOAD exists in stash, but has no START/,
 # test -stash and -src rendering
 # todo: stderr=1 puts '-e syntax OK' into $out,
 # conceivably fouling one of the lines that are tested
-$out = runperl ( switches => ["-MO=Concise,-stash=B::Concise,-src"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,-stash=B::Concise,-src"],
 		 prog => '-e 1', stderr => 1 );
 
 like($out, qr/FUNC: \*B::Concise::concise_cv_obj/,
@@ -401,13 +404,15 @@ like($out, qr/PAD_FAKELEX_MULTI is a constant sub, optimized to a IV/,
 like($out, qr/\# 4\d\d: \s+ \$l->concise\(\$level\);/,
      "src-line rendering works");
 
-$out = runperl ( switches => ["-MO=Concise,-stash=ExtUtils::Mksymlists,-src,-exec"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,-stash=ExtUtils::Mksymlists,-src,-exec"],
 		 prog => '-e 1', stderr => 1 );
 
 like($out, qr/FUNC: \*ExtUtils::Mksymlists::_write_vms/,
      "stash rendering loads package as needed");
 
-$out = runperl ( switches => ["-MO=Concise,-stash=Data::Dumper,-src,-exec"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,-stash=Data::Dumper,-src,-exec"],
 		 prog => '-e 1', stderr => 1 );
 
 like($out, qr/FUNC: \*Data::Dumper::format_refaddr/,
@@ -416,7 +421,8 @@ like($out, qr/FUNC: \*Data::Dumper::format_refaddr/,
 my $prog = q{package FOO; sub bar { print "bar" } package main; FOO::bar(); };
 
 # this would fail if %INC used for -stash test
-$out = runperl ( switches => ["-MO=Concise,-src,-stash=FOO,-main"],
+$out = runperl ( switches => ["-Ilib", "-Iext/B",
+			      "-MO=Concise,-src,-stash=FOO,-main"],
 		 prog => $prog, stderr => 1 );
 
 like($out, qr/FUNC: \*FOO::bar/,
diff --git a/ext/B/t/showlex.t b/ext/B/t/showlex.t
index 257b9c9..c10b712 100644
--- a/ext/B/t/showlex.t
+++ b/ext/B/t/showlex.t
@@ -51,7 +51,7 @@ sub padrep {
 
 for $newlex ('', '-newlex') {
 
-    $out = runperl ( switches => ["-MO=Showlex,$newlex"],
+    $out = runperl ( switches => ["-Ilib", "-Iext/B", "-MO=Showlex,$newlex"],
 		     prog => 'my ($a,$b)', stderr => 1 );
     $na = padrep('$a',$newlex);
     $nb = padrep('$b',$newlex);
-- 
1.7.2.2

From @rurban

Jim Cromie (via RT) schrieb​:

# New Ticket Created by Jim Cromie
# Please include the string​: [perl #77536]

e​: implements 'use integer';
can you suggest perl statements that trigger these particular cases ?

I already posted a similar example with and without.
Just look for i_add vs add, and i_modulo vs modulo.

From worse to better, so you see how bad the optimiser really is.
I wouldn't really test this, because it should be kept
implementation dependent how "good" our optimizer is.

Do almost nothing, slow​: ($n is not detected as constant)

$ perl -MO=Terse -e '$n=1127;$result=($n%1000)+167772160;'
LISTOP (0x16b22a0) leave [1]
  OP (0x16b2458) enter
  COP (0x16b2258) nextstate
  BINOP (0x16b2230) sassign
  SVOP (0x16b2210) const [6] IV (0x15a2138) 1127
  UNOP (0x16b21f0) null [15]
  PADOP (0x16b21d0) gvsv GV (0x15a2150) *n
  COP (0x16b2400) nextstate
  BINOP (0x16b23d8) sassign
  BINOP (0x16b23b0) add [5]
  BINOP (0x16b2368) modulo [4]
  UNOP (0x16b2328) null [15]
  PADOP (0x16b2308) gvsv GV (0x15a2150) *n
  SVOP (0x16b2348) const [7] IV (0x15a2180) 1000
  SVOP (0x16b2390) const [8] IV (0x1543bf8) 167772160
  UNOP (0x16b22e8) null [15]
  PADOP (0x16b22c8) gvsv GV (0x15a22b8) *result

Use at least intops​:

$ perl -MO=Terse -Minteger -e '$n=1127;$result=($n%1000)+167772160;'
LISTOP (0x16b2998) leave [1]
  OP (0x16b2b50) enter
  COP (0x16b2940) nextstate
  BINOP (0x16b2918) sassign
  SVOP (0x16b2340) const [6] IV (0x15a2210) 1127
  UNOP (0x16b2400) null [15]
  PADOP (0x16b21f0) gvsv GV (0x15a2288) *n
  COP (0x16b2af8) nextstate
  BINOP (0x16b2ad0) sassign
  BINOP (0x16b2aa8) i_add [5]
  BINOP (0x16b2a60) i_modulo [4]
  UNOP (0x16b2a20) null [15]
  PADOP (0x16b2a00) gvsv GV (0x15a2288) *n
  SVOP (0x16b2a40) const [7] IV (0x15a2198) 1000
  SVOP (0x16b2a88) const [8] IV (0x16c2aa0) 167772160
  UNOP (0x16b29e0) null [15]
  PADOP (0x16b29c0) gvsv GV (0x1543be0) *result

Do some simple minor folding​:

$ perl -MO=Terse -e '$result=(1127%1000)+167772160;'
LISTOP (0x16b2370) leave [1]
  OP (0x16b2398) enter
  COP (0x16b2328) nextstate
  BINOP (0x16b2300) sassign
  SVOP (0x16b22e0) const [4] IV (0x15a2270) 167772287
  UNOP (0x16b21f0) null [15]
  PADOP (0x16b21d0) gvsv GV (0x15a2150) *result

And finally constant fold with symbols​:

$ perl -MO=Terse -Minteger -e 'use constant n=>1127;
$result=(n%1000)+167772160;'
LISTOP (0x13a0520) leave [1]
  OP (0x13786c0) enter
  COP (0x13a04c0) nextstate
  BINOP (0x13a0498) sassign
  SVOP (0x1378448) const [4] IV (0x13816a8) 167772287
  UNOP (0x1396920) null [15]
  PADOP (0x1396730) gvsv GV (0x13b09b8) *result
--
Reini Urban

The RT System itself - Status changed from 'new' to 'open'

From @jimc

On Tue, Aug 31, 2010 at 3​:50 PM, Reini Urban via RT <
perlbug-followup@​perl.org> wrote​:

Jim Cromie (via RT) schrieb​:

# New Ticket Created by Jim Cromie
# Please include the string​: [perl #77536]

e​: implements 'use integer';
can you suggest perl statements that trigger these particular cases ?

I already posted a similar example with and without.
Just look for i_add vs add, and i_modulo vs modulo.

yes - I see that now,
I'll add several tests ( -Minteger etc) to my B-Concise based test patch,
along these lines youve outlined.

Ive tested fold-constants wrt Nicks a-e comments,
core tests fail badly in basically every removal or goto nope case.
Its unclear to me why it should be your burden to say so,
nor is it clear that saying it will be sufficient.

And my "system" tests work fine as a test of internal behavior.
Itd be nice if he noted that, rather than harp on their insufficiency
for API tests, which hes against anyway.

So, to doublecheck, you still want embed with XM rather than A, correct ?

From worse to better, so you see how bad the optimiser really is.
I wouldn't really test this, because it should be kept
implementation dependent how "good" our optimizer is.

Im not sure I agree - a few failures wouldnt slow down somebody
determined enough to even start improving the peep-optimizer.
esp if theres a "not much optimization happening here" after the OK.

I wonder whats needed to catch and optimize
{ my $foo = 1; print $foo+2 }
similar to your 1st example, but with known scope
SSA for perl sounds like too much work.

Do almost nothing, slow​: ($n is not detected as constant)

$ perl -MO=Terse -e '$n=1127;$result=($n%1000)+167772160;'
LISTOP (0x16b22a0) leave [1]
OP (0x16b2458) enter
COP (0x16b2258) nextstate
BINOP (0x16b2230) sassign
SVOP (0x16b2210) const [6] IV (0x15a2138) 1127
UNOP (0x16b21f0) null [15]
PADOP (0x16b21d0) gvsv GV (0x15a2150) *n
COP (0x16b2400) nextstate
BINOP (0x16b23d8) sassign
BINOP (0x16b23b0) add [5]
BINOP (0x16b2368) modulo [4]
UNOP (0x16b2328) null [15]
PADOP (0x16b2308) gvsv GV (0x15a2150) *n
SVOP (0x16b2348) const [7] IV (0x15a2180) 1000
SVOP (0x16b2390) const [8] IV (0x1543bf8) 167772160
UNOP (0x16b22e8) null [15]
PADOP (0x16b22c8) gvsv GV (0x15a22b8) *result

Use at least intops​:

$ perl -MO=Terse -Minteger -e '$n=1127;$result=($n%1000)+167772160;'
LISTOP (0x16b2998) leave [1]
OP (0x16b2b50) enter
COP (0x16b2940) nextstate
BINOP (0x16b2918) sassign
SVOP (0x16b2340) const [6] IV (0x15a2210) 1127
UNOP (0x16b2400) null [15]
PADOP (0x16b21f0) gvsv GV (0x15a2288) *n
COP (0x16b2af8) nextstate
BINOP (0x16b2ad0) sassign
BINOP (0x16b2aa8) i_add [5]
BINOP (0x16b2a60) i_modulo [4]
UNOP (0x16b2a20) null [15]
PADOP (0x16b2a00) gvsv GV (0x15a2288) *n
SVOP (0x16b2a40) const [7] IV (0x15a2198) 1000
SVOP (0x16b2a88) const [8] IV (0x16c2aa0) 167772160
UNOP (0x16b29e0) null [15]
PADOP (0x16b29c0) gvsv GV (0x1543be0) *result

Do some simple minor folding​:

$ perl -MO=Terse -e '$result=(1127%1000)+167772160;'
LISTOP (0x16b2370) leave [1]
OP (0x16b2398) enter
COP (0x16b2328) nextstate
BINOP (0x16b2300) sassign
SVOP (0x16b22e0) const [4] IV (0x15a2270) 167772287
UNOP (0x16b21f0) null [15]
PADOP (0x16b21d0) gvsv GV (0x15a2150) *result

And finally constant fold with symbols​:

$ perl -MO=Terse -Minteger -e 'use constant n=>1127;
$result=(n%1000)+167772160;'
LISTOP (0x13a0520) leave [1]
OP (0x13786c0) enter
COP (0x13a04c0) nextstate
BINOP (0x13a0498) sassign
SVOP (0x1378448) const [4] IV (0x13816a8) 167772287
UNOP (0x1396920) null [15]
PADOP (0x1396730) gvsv GV (0x13b09b8) *result
--
Reini Urban

From @doy

I'm not really sure what's going on here, because there seems to be some
context missing. Is there anything that needs to be done here?

-doy

From @rurban

On Thu, Jul 5, 2012 at 2​:34 AM, Jesse Luehrs via RT
<perlbug-followup@​perl.org> wrote​:

I'm not really sure what's going on here, because there seems to be some
context missing. Is there anything that needs to be done here?

Two things are missing​:
1. fold_constants is still not public, so it cannot be used in
external compile-time optimizers.

Nick's argumentations against it was
- that he does not understand it (probably that's why he broke it
against common agreement with by removing i_opt about a decade ago,
instead of overflow checking),
- that it needs tests (which jim cromie provided),
- and that it is not usable (this particular argumentation was
completely wrong).

Nick also said that he tries to stay away from things he does not understand.

2. [RT #81332] Nick moved XSLoader​::load B into the BEGIN block, thus
polluting the resulting compiled code with all B constants which makes
esp. the Bytecode compiler unattractive to use.
--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

From @nwc10

On Sun, Jul 08, 2012 at 10​:42​:25AM -0500, Reini Urban wrote​:

On Thu, Jul 5, 2012 at 2​:34 AM, Jesse Luehrs via RT
<perlbug-followup@​perl.org> wrote​:

I'm not really sure what's going on here, because there seems to be some
context missing. Is there anything that needs to be done here?

Yes. I have asked questions which *remain* unanswered. I shall restate them,
and clarify where I feel Reini's summary of my thoughts/understanding isn't
best described by the words he used.

I remain of the strong opinion that the changes requested have not been
justified, because the open questions make it clear that it is not obvious
that the changes are valid changes to make.

Two things are missing​:
1. fold_constants is still not public, so it cannot be used in
external compile-time optimizers.

Nick's argumentations against it was
- that he does not understand it (probably that's why he broke it

No, I understand it well enough to see that it's actually complex enough that
it's not obvious what it does. To restate my initial question which you
continue not to answer​:

http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2010-08/msg00920.html

  You patch has no tests. For the documentation for fold_constants I see​:
 
  -static OP *
  -S_fold_constants(pTHX_ register OP *o)
  +/*
  +=for apidoc Xpd|OP*|fold_constants|NN OP *o
  +
  +Fold constants under this op. This is run after constructing
  +UNOPs and BINOPs and in the ck_select check routines.
  +See L<perlguts/"Compile pass 1a​: constant folding">.
  +
  +=cut
  +*/
  +OP *
  +Perl_fold_constants(pTHX_ register OP *o)
 
 
  So does that mean that it's valid in future for this function to stick to
  *that* documentation, and only fold constants?
 
  As I discovered by accident last night, it also
 
  b​: creates targets for ops that need targets but don't yet have them
 
  and I see from looking at the code it also
 
  c​: propagates scalar context for ops that return a scalar
  d​: runs LINKLIST()
  e​: implements 'use integer';
 
 
  As it's not documented as doing any of those, is it OK for a future version of
  perl to stop doing those? (Which might happen as a side effect of refactoring)

And I should now note that it no longer does all of the above.

It now only runs LINKLIST() and folds constants.

Is that the behaviour that B​::Generate requires? Or does it require the
original 5? Or does it *just* want constants to be folded?

against common agreement with by removing i_opt about a decade ago,
instead of overflow checking),

There was common agreement - that it was a bug. Do you have any URLs to
show disagreement with this, let alone "common agreement" that the
optimisation is valid?

To restate - the "optimisation", present until 5.6.0, was that the peephole
optimiser would substitute the "integer" ops (ie the ops used under
use integer;) in place of the standard arithmetic ops, in places where it
thought that it was safe to do so, because all the constants and functions
involved only returned integers. So​:

http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2010-08/msg01025.html

  $ /export/home/nwc10/Reference/5.005_03/bin/perl5.00503 -e 'my $a = "AB"; my $int = 0x7FFFFFFF + length $a + 0x7FFFFFFF; warn $int'
-2147483639 at -e line 1.
  $ /export/home/nwc10/Reference/5.005_03/bin/perl5.00503 -e 'my $a = "AB"; my $int = 0.1 + 0x7FFFFFFF + length $a + 0x7FFFFFFF; warn $int'
2147483657.1 at -e line 1.

Similarly the return result of time is an integer, so it was assumed to be
safe to use integer multiply on its result. Result​:

$ /export/home/nwc10/Reference/5.005_03/bin/perl5.00503 -le 'print time; print time * 1000'
1341932915
1903118648

whereas the expected answer would be something like this​:

$ perl -le 'print time; print time * 1000'
1341932959
1341932959000

which 5.6.0 and later on a 32 bit system give. Because they don't have
i_opt.

If I understood you correctly at the time (sorry, can't find the message),
you were stating these two should be different in Perl​:

$ perl -le 'print 4294967297 * 4294967295'
18446744073709551615
$ perl -le 'print 4294967297.0 * 4294967295.0'
18446744073709551615

because the former is an integer expression, whereas the latter is a floating
point expression, so you were stating that the former ought to be folded using
integer maths. (Which, clearly currently, it is not)

As I stated back then​:

http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2010-08/msg01020.html

  I don't agree. I don't think that it's safe. I can't *prove* that it's safe,
  in the general case. And my hunch is that the amount of time making range
  checks, and the end user cost of bugs caused by bad range checks, is going
  to be higher than the speed gains of any optimisations.

An optimisation is only an optimisation if it is both

a) correct
b) faster

than what it replaces.

Moreover, buggy optimisations cost far far more people time than they save.

I'm not convinced that it's possible to prove that this is a safe
optimisation. And I am suspicious that it's hard to *execute* range checks
in less time than then *net* gain in code runtime. In particular, this is
because in ANSI C

a) signed integer overflow is undefined behaviour
b) there is no guarantee that an unsigned type twice the size of IVs is
  available to do the maths unsigned
c) unlike addition or subtraction, you can't spot that you've wrapped with
  just the bottom half of the multiplication

- that it needs tests (which jim cromie provided),

No, as I stated at the time, it needed unit tests. Jim provided tests that
verified that the perl parser folds constants in the optrees.

What is needed are tests that calling the function fold_constants() at the C
level does do what the documentation says that it does - fold constants.

(Or, as above - whatever it is that "fold constants" is supposed to do,
given that it does more than just what it is currently documented to do)

The current tests do not verify that the function behaves. It would be
possible to move the guts of it elsewhere, replace it with a null stub
and nothing would fail. Hence it is NOT tested currently.

- and that it is not usable (this particular argumentation was
completely wrong).

No, that it is should not be called at runtime. More specifically, that it
should not be called unless PL_curcop != &PL_compiling - ie that it's an
undocumented precondition of the function that PL_curcop == &PL_compiling,
which all its callers currently comply with. It's obvious that it is making
this assumption, as the code ends​:

  PL_warnhook = oldwarnhook;
  PL_diehook = olddiehook;
  PL_curcop = &PL_compiling;

which would put the interpreter in an inconsistent state if the function
had been entered with PL_curcop set to anything other than PL_compiling.

To restate a previous message on *exactly* this subject​:

http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2009-07/msg01272.html

  Given this​:
 
  #define IN_PERL_COMPILETIME (PL_curcop == &PL_compiling)
  #define IN_PERL_RUNTIME (PL_curcop != &PL_compiling)
 
  and their widespread use​:
 
  gv.c​: else if (IN_PERL_COMPILETIME) {
  op.c​: if (IN_PERL_RUNTIME) {
  perl.h​: (IN_PERL_COMPILETIME ? IN_LOCALE_COMPILETIME : IN_LOCALE_RUNTIME)
  pp_ctl.c​: if (IN_PERL_COMPILETIME) {
  pp_ctl.c​: runtime = IN_PERL_RUNTIME;
  pp_ctl.c​: PUSHBLOCK(cx, CXt_EVAL|(IN_PERL_COMPILETIME ? 0 : CXp_REAL), SP);
  pp_ctl.c​: if (IN_PERL_COMPILETIME)
  regcomp.c​: && IN_PERL_RUNTIME)
  regcomp.c​: if (IN_PERL_COMPILETIME)
  sv.c​: else if (IN_PERL_RUNTIME)
  sv.c​: else if (IN_PERL_RUNTIME)
  sv.c​: && IN_PERL_RUNTIME
  sv.c​: if (IN_PERL_COMPILETIME) {
  sv.c​: if (IN_PERL_RUNTIME)
  sv.c​: if (IN_PERL_RUNTIME)
  utf8.c​: if (IN_PERL_COMPILETIME) {
  utf8.c​: if (IN_PERL_COMPILETIME)
 
 
  I don't believe that it's actually *legal* to build ops at runtime.
  Specifically, code that wants to build ops should "switch to compile time"
  for the duration of op-building, and switch back once finished.
  Otherwise the behaviour of the internals isn't going to be correct.
 
  I'm not sure what is involved in "switch to compile time". It may be as simple
  as re-pointing PL_curcop at PL_compiling, and ensuring that PL_compiling has
  the appropriate dynamic hints flags set I've not checked, but I assume that
  the code for the eval op shows the way to go on this. (And quite possibly,
  might have code that should be refactored out into a new piece of the API)

To call these functions properly, the interpreter state must be "compile time".
The assertion is not a hack intended to make life awkward - it's enforcing
an assumption of the interface. And as I stated in that message, it's
perfectly possible to switch between the two states, but *how* to do so isn't
clearly documented or encapsulated.

Furthermore, the reason that I find the entire subject of B​::Generate
troubling is that it's pretty clear that it can NEVER work well.

http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2010-08/msg00988.html

  So I commented out the assertion, and rebuilt perl. I've added runloop tracing​:
 
  Breakpoint 1, XS_B__OP_convert (cv=0xb69260) at lib/B/Generate.c​:1267
  1267 dVAR; dXSARGS;
  (gdb) c
  Continuing.
  (../generate.pl​:0) pushmark
  (../generate.pl​:0) const(IV(42))
  (../generate.pl​:0) sprintf
 
  Program received signal SIGSEGV, Segmentation fault.
  0x00000000005644cc in Perl_pp_entersub () at pp_hot.c​:2939
  2939 return NORMAL;
  (gdb) p PL_op
  $9 = (OP *) 0x0
 
 
  *If* fold_constants needs to fold constants, it runs an inferior runloop.
  At the end of that, PL_op is NULL.
  B​::Generate makes no attempt to save PL_op.
  Hence I can't see how it *ever* worked.
 
 
 
  Even with the above solved, from looking at the code, and thinking about how
  it works, there are three more problems I can see with it.
 
  1​: For your example of a binop, if I ask B​::Generate to generate
  my $six = B​::SVOP->new("const", 0, 6);
  my $nine = B​::SVOP->new("const", 0, 9);
 
  my $product = B​::BINOP->new("multiply", 0, $six, $nine);
 
  then what it returns will actually be an OP_CONST, but the XS code will
  Bless it into the class B​::BINOP. That's not right. BINOPs are different
  structures, so some XS wrappers for BINOPs are going to return garbage or
  SEGV when used on an SVOP.
 
  This one seems to be fairly easily fixable, but it will cause some surprises
  if people always expect the class of the object returned to match the
  class of constructor called
 
 
  2​: For the example above, *what* do $six and $nine point to after the last
  line?
 
  If I read the code correctly, if constants have been folded, this line is
  reached in fold_constants()
 
  op_free(o);
 
  and the entire original optree is freed. At which point, $six and $nine are
  corrupt. Internally, they still point to the same addresses as ever. But
  that's now freed memory, and they have no way of knowing.
 
  I can't see an easy solution to this.
 
  3​: There is no DESTROY method. Any ops you create leak, unless you
  successfully attach them to somewhere appropriate (which used to have a
  NULL pointer, else you've just shifted the leak around)
 
 
  So right now, I'm really not sure how useful B​::Generate actually is.
  It's adding a layer of indirection to the C internals, rather than providing
  a layer of abstraction. You end up having to write C in, well more, *through*
  Perl. With no control over memory management.
 
  I'm half thinking that if B​::Generate wants to fold constants, it would be
  better providing the constant folding algorithm in Perl. At least that way
  it would keep control over the B​:: objects that it is manipulating.
 
  But the other half of me isn't sure if it's worth it. As-is, you have to
  understand the C structures to write ops. I wrote a new peephole optimisation
  on Friday in a few hours. In C, with some help from *printf, gdb and valgrind.
  I think that it would actually have been *harder* to try to do it via an
  OO interface in Perl, even without bugs.

So, specifically, because B​::Generate has no DESTROY method, and because of
how it works, can't have a DESTROY method. So

1​: Any ops that B​::Generate creates will (by default) *leak*
2​: Any manipulation invoked by B​::Generate may cause existing OPs to be
  *freed*, leaving Perl datastructures pointing to freed memory.
  (Undefined behaviour, potentially heap smashing)

plus

3​: It provides *no abstraction* from the C level manipulations
4​: It is *undocumented* how to use it (because the core doesn't document how
  optrees are supposed to fit together)

hence I continue to assert that the approach it takes can't work.
(I am happy to be proven wrong with evidence and reasoning. None has yet
been forthcoming.)

Nick also said that he tries to stay away from things he does not understand.

People should not be changing things that they don't understand.
People should also be aware of where the limits of their understanding are.

Hence *no-one* should be changing the code unless they understand it.

I have asked for clarification on areas that I know I don't understand.
None has been forthcoming from anyone. If this remains the case, then I
contest that it is sensible to assume that no-one understands it.

Nicholas Clark

From @doy

On Tue, Jul 10, 2012 at 05​:04​:10PM +0100, Nicholas Clark wrote​:

You patch has no tests\. For the documentation for fold\_constants I see​:

\-static OP \*
\-S\_fold\_constants\(pTHX\_ register OP \*o\)
\+/\*
\+=for apidoc Xpd|OP\*|fold\_constants|NN OP \*o
\+
\+Fold constants under this op\. This is run after constructing 
\+UNOPs and BINOPs and in the ck\_select check routines\.
\+See L\<perlguts/"Compile pass 1a&#8203;: constant folding">\.
\+
\+=cut
\+\*/
\+OP \*
\+Perl\_fold\_constants\(pTHX\_ register OP \*o\)


So does that mean that it's valid in future for this function to stick to
\*that\* documentation\, and only fold constants?

As I discovered by accident last night\, it also

b&#8203;: creates targets for ops that need targets but don't yet have them

and I see from looking at the code it also

c&#8203;: propagates scalar context for ops that return a scalar
d&#8203;: runs LINKLIST\(\)
e&#8203;: implements 'use integer';


As it's not documented as doing any of those\, is it OK for a future version of
perl to stop doing those? \(Which might happen as a side effect of refactoring\)

And I should now note that it no longer does all of the above.

It now only runs LINKLIST() and folds constants.

On a related note, I would very much like to see #11606 be finished at some
point, because it would make manual optree creation and debugging much
easier. From the comments in that ticket, it sounds like getting it to
work really would involve removing all extra behaviors from
fold_constants.

-doy

From @rjbs

* Nicholas Clark <nick@​ccl4.org> [2012-07-10T12​:04​:10]

People should not be changing things that they don't understand.
People should also be aware of where the limits of their understanding are.

Hence *no-one* should be changing the code unless they understand it.

Alternately, "I made some changes and it did what I wanted! I have no idea
why! Help! It's here on this branch..."

That's actually often going to be a lot better than "I didn't know why it
worked so I deleted it and gave up."

Also, better than, "I didn't know why it worked, and neither did the guy who
committed it, but we figured it can't hurt, right?"

--
rjbs