p5pRT
commented
11 years ago From @bulk88
Created by @bulk88
See attached patch.
Closed p5pRT closed 11 years ago
p5pRT
commented
11 years ago See attached patch.
p5pRT
commented
11 years ago p5pRT
commented
11 years ago On Sat\, Oct 27\, 2012 at 4:22 AM\, bulk 88 \perlbug\-followup@​perl\.org wrote:
# New Ticket Created by bulk 88 # Please include the string: [perl #115496] # in the subject line of all future correspondence about this issue. # \<URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115496 >
This is a bug report for perl from bulk88@hotmail.com\, generated with the help of perlbug 1.39 running under perl 5.17.6.
----------------------------------------------------------------- [Please describe your issue here]
See attached patch.
And the patch inline for ready reference looks like:
From d95498923127ccd1e24ef3a151f49c6fb5652d54 Mon Sep 17 00:00:00 2001 From: Daniel Dragan \bulk88@​hotmail\.com Date: Sat\, 27 Oct 2012 01:44:13 -0400 Subject: [PATCH] rmv a sv_2mortal and unused var in toke.c:Perl_yyerror_pvn
newSVpvn_flags is capable of mortalizing already\, use that\, is_utf8 is used only once\, waste of an auto var stack slot to calculate it so early\, instead create the flags arg to newSVpvn_flags at the point of usage. flags param of yyerror_pvn will always be on the C stack.
toke.c | 3 +-- 1 files changed\, 1 insertions(+)\, 2 deletions(-)
STRLEN len, U32 flags) SV *msg; SV * const where_sv = newSVpvs_flags(""\, SVs_TEMP); int yychar = PL_parser->yychar; - U32 is_utf8 = flags & SVf_UTF8;
PERL_ARGS_ASSERT_YYERROR_PVN;
@@ -11098\,7 +11097\,7 @@ Perl_yyerror_pvn(pTHX_ const char *const s\, STRLEN len\, U32 flags) else Perl_sv_catpvf(aTHX_ where_sv\, "\\%03o"\, yychar & 255); } - msg = sv_2mortal(newSVpvn_flags(s\, len\, is_utf8)); + msg = newSVpvn_flags(s\, len\, (flags & SVf_UTF8) | SVs_TEMP); Perl_sv_catpvf(aTHX_ msg\, " at %s line %"IVdf"\, "\, OutCopFILE(PL_curcop)\, (IV)CopLINE(PL_curcop)); if (context) -- 1.7.9.msysgit.0
Note that sv_2mortal does a couple of checks before setting the SVs_TEMP bit -- I didn't see any rationale for why those checks are superfluous here but maybe it's obvious and I missed it.
As far as removing the is_utf8 variable\, I doubt that change will do anything an optimizing compiler wouldn't already do\, though I could be wrong and that could probably be demonstrated by determining whether the object code is the same before and after. I agree that for its single use it's not really earning its keep in enhanced readability.
p5pRT
commented
11 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
11 years ago RT Web refused to quote your post for me below the inline patch file you pasted (RT bug?) so my quotes below were made by hand. _______________________________________________________ On Sat Oct 27 16:14:26 2012\, craig.a.berry@gmail.com wrote: Note that sv_2mortal does a couple of checks before setting the SVs_TEMP bit -- I didn't see any rationale for why those checks are superfluous here but maybe it's obvious and I missed it. _______________________________________________________ newSVpvn_flags will call sv_2mortal for you if you ask it with a constant. Since the branch in newSVpvn_flags on whether to mortal the SV will be evaluated no matter what\, we might as well use it with a simple unconditional or immediate asm op. _______________________________________________________ On Sat Oct 27 16:14:26 2012\, craig.a.berry@gmail.com wrote: As far as removing the is_utf8 variable\, I doubt that change will do anything an optimizing compiler wouldn't already do\, though I could be wrong and that could probably be demonstrated by determining whether the object code is the same before and after. I agree that for its single use it's not really earning its keep in enhanced readability. __________________________________________________________________ GCC didn't do it and neither did Visual C (my usual compiler). Can you show the machine code of a C compiler that would reorder it automatically?
Here is yyerror_pvn's header in Strawberry Perl 5.16\, which is GCC
___________________________________________________________________
7139FCB0 55 push ebp
7139FCB1 57 push edi
7139FCB2 56 push esi
7139FCB3 53 push ebx
7139FCB4 83 EC 4C sub esp\,4Ch
7139FCB7 8B 5C 24 60 mov ebx\,dword ptr [esp+60h] ;my_perl
7139FCBB 8B 6C 24 6C mov ebp\,dword ptr [esp+6Ch] ;arg 4\,flags
7139FCBF C7 44 24 0C 00 00 08 00 mov dword ptr [esp+0Ch]\,80000h
7139FCC7 C7 44 24 08 00 00 00 00 mov dword ptr [esp+8]\,0
7139FCCF C7 44 24 04 F0 23 40 71 mov dword ptr [esp+4]\,714023F0h
7139FCD7 89 1C 24 mov dword ptr [esp]\,ebx ;my_perl for
Perl_newSVpvn_flags
7139FCDA 81 E5 00 00 00 20 and ebp\,20000000h ;UTF flag
calculated into a non-volatile register
7139FCE0 E8 AB 49 FF FF call 71394690 ; Perl_newSVpvn_flags
_______________________________________________________________
The important thing is\, it was not optimized or reordered\, unless done by hand. Why\, IDK enough to say\, maybe someone else can answer. The words varargs and aliasing on x86 I think are involved in the full answer. If you can pick a GCC option that would reorder the calculation of is_utf8 (it probably exists knowing GCC's huge highly detailed optimization switches)\, why didn't Strawberry Perl come with it on by default?
Visual C put is_utf8 on the C stack\, GCC saved a non-vol reg\, then put is_utf8 into the non-vol reg. At the end\, they did the same thing\, put 1 extra thing on the C stack they didn't have to at that point. Also neither did "flags = flags & UTF8FLAG;" and changed the incoming flags param even though flags var is not used in the body of yyerror_pvn ever again. To do that by hand I think would make the code unmaintainable\, do you agree?
p5pRT
commented
11 years ago On Sat Oct 27 16:14:26 2012\, craig.a.berry@gmail.com wrote:
Note that sv_2mortal does a couple of checks before setting the SVs_TEMP bit -- I didn't see any rationale for why those checks are superfluous here but maybe it's obvious and I missed it.
Those two checks are a null check and then an immortality check. Neither can be true for a newly-created SV.
--
Father Chrysostomos
p5pRT
commented
11 years ago On Sat Oct 27 16:14:26 2012\, craig.a.berry@gmail.com wrote:
On Sat\, Oct 27\, 2012 at 4:22 AM\, bulk 88 \perlbug\-followup@​perl\.org wrote:
# New Ticket Created by bulk 88 # Please include the string: [perl #115496] # in the subject line of all future correspondence about this issue. # \<URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115496 >
This is a bug report for perl from bulk88@hotmail.com\, generated with the help of perlbug 1.39 running under perl 5.17.6.
----------------------------------------------------------------- [Please describe your issue here]
See attached patch.
And the patch inline for ready reference looks like:
From d95498923127ccd1e24ef3a151f49c6fb5652d54 Mon Sep 17 00:00:00 2001 From: Daniel Dragan \bulk88@​hotmail\.com Date: Sat\, 27 Oct 2012 01:44:13 -0400 Subject: [PATCH] rmv a sv_2mortal and unused var in toke.c:Perl_yyerror_pvn
newSVpvn_flags is capable of mortalizing already\, use that\, is_utf8 is used only once\, waste of an auto var stack slot to calculate it so early\, instead create the flags arg to newSVpvn_flags at the point of usage. flags param of yyerror_pvn will always be on the C stack.
Wrong ticket. That one is from #115490.
--
Father Chrysostomos
p5pRT
commented
11 years ago On Sat Oct 27 22:30:13 2012\, bulk88 wrote:
newSVpvn_flags will call sv_2mortal for you if you ask it with a constant. Since the branch in newSVpvn_flags on whether to mortal the SV will be evaluated no matter what\, we might as well use it with a simple unconditional or immediate asm op.
Rereading what Father C wrote\, PUSH_EXTEND_MORTAL__SV_C in newSVpvn_flags is not a macro for sv_2mortal\, so that sentence I wrote was wrong.
p5pRT
commented
11 years ago On Sat Oct 27 02:22:02 2012\, bulk88 wrote:
This is a bug report for perl from bulk88@hotmail.com\, generated with the help of perlbug 1.39 running under perl 5.17.6.
----------------------------------------------------------------- [Please describe your issue here]
See attached patch.
Thank you. Applied as c06ecf4f380.
--
Father Chrysostomos
p5pRT
commented
11 years ago @cpansprout - Status changed from 'open' to 'resolved'
p5pRT
commented
11 years ago "Craig A. Berry" \craig\.a\.berry@​gmail\.com wrote: :On Sat\, Oct 27\, 2012 at 4:22 AM\, bulk 88 \perlbug\-followup@​perl\.org wrote: [...] :diff --git a/toke.c b/toke.c :index 97b2170..416c952 100644 :--- a/toke.c :+++ b/toke.c :@@ -11037\,7 +11037\,6 @@ Perl_yyerror_pvn(pTHX_ const char *const s\, :STRLEN len\, U32 flags) : SV *msg; : SV * const where_sv = newSVpvs_flags(""\, SVs_TEMP); : int yychar = PL_parser->yychar; :- U32 is_utf8 = flags & SVf_UTF8; : : PERL_ARGS_ASSERT_YYERROR_PVN; : :@@ -11098\,7 +11097\,7 @@ Perl_yyerror_pvn(pTHX_ const char *const s\, :STRLEN len\, U32 flags) : else : Perl_sv_catpvf(aTHX_ where_sv\, "\\%03o"\, yychar & 255); : } :- msg = sv_2mortal(newSVpvn_flags(s\, len\, is_utf8)); :+ msg = newSVpvn_flags(s\, len\, (flags & SVf_UTF8) | SVs_TEMP); : Perl_sv_catpvf(aTHX_ msg\, " at %s line %"IVdf"\, "\, : OutCopFILE(PL_curcop)\, (IV)CopLINE(PL_curcop)); : if (context) [...] :As far as removing the is_utf8 variable\, I doubt that change will do :anything an optimizing compiler wouldn't already do\, though I could be :wrong and that could probably be demonstrated by determining whether :the object code is the same before and after. I agree that for its :single use it's not really earning its keep in enhanced readability.
I think this change actually enhances correct readability\, by removing a subtle lie - the name is_utf8 implies a boolean value\, but it is actually a bitmask\, and relies on being passed in to newSVpvn_flags that way.
Hugo
Migrated from rt.perl.org#115496 (status was 'resolved')
Searchable as RT115496$