ParseXS: heavily refactor generate_output() #22904
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonycoz
reviewed
Jan 13, 2025
dist/ExtUtils-ParseXS/t/001-basic.t
Outdated
| @@ -3089,6 +3089,26 @@ EOF | |||
| "ST(i) set" ], | |||
| [ 0, 1, qr/DO_ARRAY_ELEM/, "no DO_ARRAY_ELEM" ], | |||
| ], | |||
|
|
|||
| # for OUT and OUTLIST arguments, don't process DO | |||
There was a problem hiding this comment.
s/DO$/DO_ARRAY_ELEM/ presumably
|
Each individual commit builds and tests correctly. I haven't examined the individual commits. |
tonycoz
reviewed
Jan 16, 2025
| # OUTLIST param - push after ST(0) new mortal(s) containing the | ||
| # current values of the local var set from that | ||
| # parameter. | ||
| # OUTLIST param - ($out_num is +ve) Push after any RETVAL, new |
There was a problem hiding this comment.
$out_num can be 0 if the return type is void.
ExtUtils::ParseXS::Node::Param::as_output_code(lib/ExtUtils/ParseXS/Node.pm:523):
523: my ExtUtils::ParseXS::Node::Param $self = shift;
DB<3> b 731
DB<4> c
ExtUtils::ParseXS::Node::Param::as_output_code(lib/ExtUtils/ParseXS/Node.pm:731):
731: if ($var ne 'RETVAL' and not defined $out_num) {
DB<4> p $out_num
0
DB<5> p $var
a
tonycoz
reviewed
Jan 16, 2025
| \s* | ||
| \( \s* | ||
| $target # arg 1: SV to set | ||
| , \s* | ||
| $bal_no_comma # arg 2: value to use | ||
| | | ||
| # 3-arg functions | ||
| sv_setpvn | ||
| sv_set(?:pvn|pvn_mg|pv_bufsize) |
There was a problem hiding this comment.
From what I can see this will only match if the whole typemap entry is the appropriate call.
For sv_setpv_bufsize() this would result in a returned POK SV where the length bytes haven't been set, ie. incomplete code, so I don't see it as reasonable to optimize for.
tonycoz
reviewed
Jan 16, 2025
dist/ExtUtils-ParseXS/t/001-basic.t
Outdated
| 'foo(IN_OUTLIST int A = 0)', | ||
| ], | ||
| [ 0, 0, qr/\bXSprePUSH;/, "XSprePUSH" ], | ||
| [ 0, 0, qr/\b\QEXTEND(SP,2);/, "extend 3" ], |
|
On Wed, Jan 15, 2025 at 09:07:54PM -0800, Tony Cook wrote:
# 3-arg functions
- sv_setpvn
+ sv_set(?:pvn|pvn_mg|pv_bufsize)
>From what I can see this will only match if the *whole* typemap entry is the appropriate call.
For sv_setpv_bufsize() this would result in a returned POK SV where the `length` bytes haven't been set, ie. incomplete code, so I don't see it as reasonable to optimize for.
Ok, I'll remove sv_setpv_bufsize.
…--
"Strange women lying in ponds distributing swords is no basis for a system
of government. Supreme executive power derives from a mandate from the
masses, not from some farcical aquatic ceremony."
-- Dennis, "Monty Python and the Holy Grail"
|
|
On Wed, Jan 15, 2025 at 08:29:44PM -0800, Tony Cook wrote:
+ # OUTLIST param - ($out_num is +ve) Push after any RETVAL, new
`$out_num` can be 0 if the return type is void.
Presumably you'd be happy if the comment was changed to
"($out_num is >=0)"?
…--
My get-up-and-go just got up and went.
|
iabyn
force-pushed
the
davem/xs_refactor9
branch
from
728cdf3 to
c6a18e7
Compare
|
I've rebased and repushed, with four new commits added at the end. The first two of those should address the issues raised so far, and the final two fix a bug I spotted while addressing the $out_num issue and add 'done_testing' so I don't have to keep updating the number of tests. |
There is currently only a single test for the special-cased return type "array(type,nitems)" - which returns a packed string containing nitems of type, on the assumption that RETVAL has been assigned a pointer to an array of type[nitems]. So this commit adds more comprehensive tests.
The T_ARRAY typemap input and output code relies on a special token, DO_ARRAY_ELEM, which is converted by ParseXS into code to get/set each element of an array. This commit adds several tests for this under-tested misfeature. A few of the tests are marked with 'XXX' comments where I explain that they test the *current* behaviour, not necessarily the correct or best behaviour.
Both INPUT and OUTPUT typemap template processing expand the text
token 'DO_ARRAY_ELEM'.
In particular, input processing has code like:
if ($expr =~ /DO_ARRAY_ELEM/) {
...
$expr =~ s/DO_ARRAY_ELEM/$subexpr/;
}
and output processing has code like:
if ($expr =~ /DO_ARRAY_ELEM/) {
...
$expr =~ s/DO_ARRAY_ELEM\n/$subexpr/;
}
Note that there is a \n in the pattern only in the second substitution.
This commit does two things. First it removes the extraneous \n, which
one assumes was a mistake; and secondly it wraps all four patterns with
\b so the token only matches as a whole word. This is very unlikely to
make any difference in the real world, as DO_ARRAY_ELEM is an
undocumented hack used only in the T_ARRAY typemap entry supplied with
perl, and not anywhere on CPAN AFAICT.
In a slight quirk, perl's INPUT and OUTPUT lines for T_ARRAY which
contain DO_ARRAY_ELEM are, respectively:
DO_ARRAY_ELEM;
DO_ARRAY_ELEM
The latter of which would now match (but wouldn't before) even if it had
a trailing ';'.
This commit also does a minor fixup to a test I added in the previous
commit, which had to temporarily not have any characters after the
DO_ARRAY_ELEM in the typemap due to the \n.
DO_ARRAY_ELEM is a text token which can appear in typemap INPUT and
OUTPUT code templates. It expands into code which sets the value of one
SV. It is intended for use within the T_ARRAY typemap, which for OUTPUT
includes DO_ARRAY_ELEM within a loop which stores new mortal SVs at
stack locations ST(0)..ST(n-1) and sets each SV to the value of each
element of RETVAL.
This makes no sense when applied to an OUT or OUTLIST parameter (as
opposed to RETVAL). The code gets emitted, but just interferes with the
code to return RETVAL. For example, in something like
int
foo(OUTLIST intARRAY* x)
code will be emitted to store temp SVs at ST(0), ST(1), ... containing
the values of x[0], x[1], ...; then code will also be emitted to store
the value of RETVAL at ST(0).
For OUT params it makes even less sense: an OUT param should update one
of the SVs passed as an argument, and shouldn't be pushing *anything*
onto the stack.
This commit makes it a compile error for the token 'DO_ARRAY_ELEM' to
appear in an OUTPUT typemap for a use other than returning RETVAL.
The special type 'array(some_type.nitems) is intended for use only as the return type of an XSUB. Make using it to return other variables an error. Previously it was allowed, but generated broken C code.
Add an "internal error" croak if generate_output() tries to update the value of a passed argument whose arg num is undefined. This should never happen. Previously there was an oblique check for this, with the function silently returning if detected. This commit makes it more obvious what's (not) going on. There are no tests added because - ahem - this should ever happen.
The last statement in generate_output() is a big four-limb if/else to handle the main types of variable value return. This commit splits the first branch, if ($expr =~ /\bDO_ARRAY_ELEM\b/), into a separate if(), with an added 'return' but no else; leaving the main if/else as only three limb. This is a step towards making the DO_ARRAY_ELEM processing just part of typemap processing. Should be no change in functionality.
When there's custom return code specified on an OUTPUT line, use that rather than the code from the typemap OUTPUT entry: even if that template code includes the DO_ARRAY_ELEM special token.
In generate_output(), rather than setting $arg to "ST(n)" and later amending it for OUTLIST parameters, just set it to the right value initially. Should make no functional change, but just simplifies things slightly.
This restores the original behaviour broken during recent refactoring,
and adds tests.
In something like
array(int,5)
foo()
OUTPUT:
RETVAL my_setintptr(ST(0), RETVAL);
the special processing of the array(...) return type should be ignored
and instead the override code specified on the OUTPUT line should be
used.
This commit includes some duplicate cut and pasted perl code to return
the override; this will be refactored away shortly.
Currently when an OUTPUT line includes a code override, the type is still looked in the typemap (even though the result won't be used). If the type of RETVAL is unknown, this causes an error. This commit adds a test for this error, mainly so that the next commit, which fixes this issue, can be seen to fix the test.
Reorganise the typemap lookup code so that it's only called when
there isn't a return typemap override. For example:
blah
foo()
OUTPUT:
RETVAL some_override_code(...)
Formerly it looked up 'blah' to get an OUTPUT typemap even though it's
value wouldn't be used and the text 'some_override_code(...)' would be
used instead. This would cause an error if 'blah' couldn't be found in
the typemap.
The diff looks quite complex mainly because the ordering of some lexical
variables has been changed: all the $type-derived vars are now grouped
together, and the 'my' declarations of some vars have been moved earlier
so they aren't wrapped in the new else block that's been put around the
typemap-lookup code.
The structure now looks like:
my $type = ...;
my $ntype = derived from $type;
etc
my $expr;
if (defined $output_code) {
$expr = $output_code;
}
elsif ($type =~ /^array\(([^,]*),(.*)\)/) {
$expr = "sv_setpvn($arg, (char *)$var, $nitems * sizeof($atype));";
}
else {
$expr = ... lookup typemap ...
}
The previous commit wrapped some code lines in an 'else' block. Reindent to match. Whitespace-only.
The array pseudo-type used for return values, e.g.
array(int,5)
foo(...)
...
Is currently completely special-cased. This commit makes so it so that
instead, it just pretends that a typemap lookup was done, and that the
output template returned was (for example):
sv_setpvn($arg, (char *)$var, 5 * sizeof(int));
Then rather than emitting code like that directly, it now continues to
the general RETVAL-processing section. This means that any current or
future optimisations will be applied to it.
The test added by this commit shows that the generated code now uses the
minor RETVALSV optimisation. (It still doesn't use TARG yet).
Move the $arg declaration and setting closer to its first use. The previous commit removed an earlier need for it. No functional changes.
Some of the refactoring earlier in this branch broke some edge cases.
This commit fixes those cases and adds tests.
In particular:
1) Optional typemap override code on OUTPUT lines is *not* eval-expanded
(unlike those on INPUT lines). So add a test for this, and don't bother
setting $expr in that case, since $expr is a meant to be a pre-eval
value. (This wasn't broken, but it wasn't tested, and setting $expr
didn't make any sense, and might cause new bugs further down the line).
2) In something like
int
foo(IN_OUTLIST int abc)
OUTPUT:
abc my_set(ST[0], RETVAL);
generate_output() is called *twice* for abc, once to update the first
arg with the current value of abc, and once to push a new mortal onto
the stack holding the current value of abc. The typemap override,
my_set(...), should only be used for the updating, not the pushing.
Add a boolean field, xsub_stack_was_reset, to the ParseXS class. This field indicates that "XSprePUSH" has been emitted. In more detail: XS has two main ways of adding return SVs to the stack. For simple single-value-return XSUBs, SP is kept pointing at the top of the current stack frame (i.e. pointing at the highest passed arg), then the return value SV is stored at the base of stack frame (ST(0) = ...), and then just before returning, the stack pointer is reset to base+1 (XSRETURN(1)). However in the presence of OUTLIST params, or where where RETVAL can be returned via the TARG optimisation, SP is instead reset to point to the base of the stack frame via "XSprePUSH", then RETVAL and any OUTLIST params are PUSH()ed onto the stack. ParseXS used to keep track of this by checking in various places whether there were any OUTLIST params. Instead this commit sets a boolean flag if XSprePUSH has been emitted, then checks that flag for any further code-emitting decisions. Currently there is no difference between the two approaches (flag or check OUTLIST count), but this commit will allow in future for things to potentially become more fluid without breaking.
Add two new boolean fields to the ParseXS class:
xsub_targ_declared indicates that a dXSTARG was emitted;
xsub_targ_usable indicates that the TARG hasn't been used yet.
Currently ParseXS emits a dXSTARG early on if the typemap for RETVAL
indicates that it is TARG-optimisable. Later, when emitting code to
return RETVAL, the same check is made, and if so, a PUSHi() or whatever
is used to return an int/whatever more cheaply using TARG.
These two flags abstract out that a bit. It makes no difference yet,
but in future it would allow:
- a dXSTARG to be emitted later even if we couldn't have known
earlier that we could use it. (We can't just always move the declaration
to later, since various XS modules assume that TARG is available.)
- if/where there are more cases where using TARG might be useful, then
we avoid any risk of accidentally using the same targ to return two
different values.
ParseXS currently generates two kinds of C code to return RETVAL.
For the general case, the code looks like (give or take some extra
messing around with sv_newmortal(), SvSETMAGIC() etc):
sv_setfoo(RETVALSV, (foo)RETVAL); /* from the typemap template */
ST(0) = RETVALSV;
XSRETURN(1);
While for the "use TARG instead of a new mortal" optimisation, it
looks like:
TARGi((foo)RETVAL); /* modified typemap template to set TARG efficiently */
XSprePUSH;
PUSHs(TARG);
XSRETURN(1);
This commit makes the TARG optimisation use the first form too. As well
as being marginally more efficient, it will help with refactoring
ParseXS: soon the TARG optimisation branch and the general branch should
be able to be mostly combined. It's also more friendly to implementing
PERL_RC_STACK eventually.
In more detail:
On entry to an XSUB, ax is set to the offset to the first arg (i.e.
ST(0)), and SP/sp is set to point to the last (topmost) arg.
During execution of the body of the XSUB, the args are left on the
stack. Then when putting return values on the stack (typically just one
- RETVAL - although rarely others may be pushed too via OUTLIST), then
the general code expands to something like:
*(PL_stack_base + ax) = RETVALSV; /* ST(0) = RETVALSV; */
PL_stack_sp = PL_stack_base + ax; /* XSRETURN(1); */
Which effectively replaces the first arg with RETVAL and then blows the
rest of the args away.
While the TARG code does:
sp = PL_stack_base + ax - 1; /* XSprePUSH; */
*++sp = TARG; /* PUSHs(TARG); */
PL_stack_sp = PL_stack_base + ax; /* XSRETURN(1); */
i.e. blow all the args away, then push RETVAL.
The resetting and incrementing of sp is unnecessary: it is just a
hangover from when the TARG optimisation used PUSHi() etc.
Note that in the presence of at least one OUTLIST param, an XSprePUSH
is still emitted. The stack is reset before any value(s) are output;
the 'ST(0) = ...' actually modifies the stack slot *above* sp, but then
'SP++' is immediately emitted after that.
In the big chunk of code in generate_output() which is responsible for
emitting the C code which creates a mortal SV and then sets its value to
RETVAL and pushes it on the stack, there is a bunch of code to emit
"SvSETMAGIC(RETVALSV);" or similar if $do_setmagic is true.
However, for RETVAL:
a) $do_setmagic should never be true; and
b) even if it was, it would be pointless to call set magic on a fresh
temporary SV.
So this commit removes all the code which handles $do_setmagic in the
RETVAL branch, and instead adds a death("Internal error: ...") for if
$do_setmagic does happen to be true.
This commit should produce no changes to the generated C code.
The next commit will handle a similar situation for setting magic when
the TARG optimisation is being used.
There is a chunk of code in generate_output() which is responsible for
emitting the C code which grabs the targ from the pad, sets its value to
RETVAL, and pushes it on the stack.
The value setting can be one of two forms:
TARGi((IV)RETVAL) /* and TARGu, TARGn */
sv_setpvn_mg(TARG, RETVAL, length(retval))
The TARGi/u/n expand to:
if (TARG is simple)
update it directly
else
sv_setiv_mg(targ, i)
and similar.
However, for RETVAL:
a) $do_setmagic should never be true; and
b) even if it was, it would be pointless to call set magic on a TARG.
So this commit changes the sv_setpv_mg() to sv_setpv() and similarly
for sv_setpvn().
Note that TARGi/u/n still calls set magic in the non-shortcut case,
but I'm not worried about the slight inefficiency, as it shouldn't happen
very often.
Note that the previous commit did the same for RETVAL in the
non-TARG-optimisation case. Unlike the previous commit, this one *does*
affect what C code is generated.
Simplify the code in generate_output() which emits C code to return the value of RETVAL. This also prepares it for some changes which will follow in the next commit. Mainly it adds the $retvar lexical, which keeps track of what C var (or expression) should be used when setting values or calling sv_2mortal() etc: i.e. one of 'RETVALSV', 'RETVAL', or 'ST(0)'. (And might be 'TARG' too in the next commit). This replaces a couple of bool flags. Should be no functional changes.
There is a block of code in generate_output() which for some typemaps of
the general form sv_set[ivnp]vn?($arg, $val), uses TARG rather than a
new mortal to return the value; and which in addition, for the [iuv]
cases uses TARGi() etc to set the value more efficiently sometimes.
Currently this is handled in an entirely separate 'if' block in
generate_output(), including doing its own: evalling the typemap,
emitting C code, and returning.
This commit changes it so that the TARG optimisation is instead handled
within the general RETVAL-handling code.
This makes the code simpler and easier to maintain.
The main differences in the old and new ways are that rather than:
- constructing and evalling a whole new typemap: e.g. ditching
'sv_setiv($arg, $val)', replacing it with 'TARGi($val, 1)' and then
evalling it to get e.g. 'TARGi((IV)RETVAL, 1)',
- we instead use the post-eval version of the standard typemap,
'sv_setiv(RETVALSV, (IV)RETVAL)', and then for the specific cases of
[iuv], apply a regex to convert it to 'TARGi((IV)RETVAL, 1)'. This
should be functionally the same, but may have minor white space
changes.
By using the standard eval code path, the typemap is evalled with the
full complement of vars such as $ntype, $num etc, rather than just $var
and $type. Whether this better or worse is debatable. Arguably the
standard eval sets too many vars (as a historical accident), while the
(just removed) TARG-specific eval arguably sets too few vars.
For the sv_setpv() and sv_setpvn() cases, we don't modify the result,
except to change 'RETVALSV' to 'TARG'. This now causes typecasts which
were in the original typemap to be kept, whereas before they were
striped. So
sv_setpv(TARG, ....)
becomes
sv_setpv((SV*)TARG, ....)
Which shouldn't make any difference since TARG is already SV*.
Apart from the white space and restoration of any cast, and the changes
in vars available to the eval, there should be no other functional
changes.
I've also added code which emits a late 'dXSTARG' if one hasn't already
been emitted earlier in the XSUB body, but for now, it should already
have been emitted earlier always. But this allows for potential future
expansions to what can use the TARG.
Recent commits have extensively messed with generate_output(). This commit updates and clean ups all the code comments in that area,` and moves a couple of lexical var declarations closer to where they're first used.
Add several tests where an OUTLIST param is the first or second value to be returned on the stack. The next commit will make OUTLIST params use the same code path as the RETVAL return code generation, so add lots of tests now to see what (if anything) changes. In theory, merging the code paths opens up the possibility of TARG being used to return an OUTLIST value for example.
In an XSUB like
int
foo(OUTLIST int A, OUTLIST int B)
...
C code is emitted to push SVs onto the stack containing the values of
RETVAL, A and B. So in principle, something like:
XSprePUSH;
EXTEND(SP,3);
PUSHs(sv_newmortal());
sv_setiv(ST(0), (IV)RETVAL);
PUSHs(sv_newmortal());
sv_setiv(ST(1), (IV)A);
PUSHs(sv_newmortal());
sv_setiv(ST(2), (IV)B);
However, RETVAL and OUTLIST params are currently handled by two separate
branches within generate_output().
The OUTLIST branch always just emits a PUSHs() / sv_setX() pair.
The RETVAL branch on the other hand includes a number of optimisations,
such as using TARG instead of sv_newmortal(), using TARGi() rather than
sv_setiv() to set the value of the SV, using SV* RETVALSV as an
intermediate variable, not copying the SV when the typemap returns an
SV, and skipping a mortalise when the typemap returns an immortal SV.
This commit deletes the OUTLIST branch, and instead makes OUTLIST params
use the RETVAL branch. This immediately makes most of the RETVAL
optimisations available to OUTLIST params. In particular, if the XSUB is
void return type, then the TARG is still available to be used for the
first OUTLIST param.
This isn't as exciting as it may first seem, as very little real XS code
actually uses OUTLIST params. But it simplifies and regularises the code
in generate_output(), and means that any future added RETVAL
optimisations will automatically apply to OUTLIST values too.
Note that some OUTPUT typemaps look like:
T_BOOL
${"$var" eq "RETVAL" ? \"$arg = boolSV($var);" : \"sv_setsv($arg, boolSV($var));"}
which for now will carry on using the inefficient sv_setsv() branch for
non-RETVAL returns, so will copy rather than returning an immortal
directly.
Add a Typemaps::OutputMap::targetable_legacy() method which is,
initially, just an exact copy of the existing targetable() method. Then
use the legacy method to determine whether to emit an early dXSTARG,
while continue to use the targetable() method to determine whether to
emit a late dXSTARG.
Background: until recently, the TARG optimisation was used specifically
only where the typemap entry for RETVAL's type was one of
sv_set{iv,uv,nv,pv,pvn}. This was detected by the targetable() method,
and if true, a dXSTARG was emitted near the start of the XSUB's body,
then the sv_setiv(ST(0)), RETVAL) or whatever near the end of the XSUB
was replaced with C<TARGi(RETVAL); PUSHs(TARG)> or similar.
Recent changes have expanded where TARG can be used, sometimes where the
usage can't be detected early. In these cases, a dXSTARG is instead
emitted in a tight scope near the end of the XSUB, e.g.
{ dXSTARG; sv_setfoo(TARG, bar); PUSHs(TARG); }
Ideally the early dXSTARG would never be emitted and dXSTARG would just
be emitted where and when it is required. However, some CPAN XS code
assumes that TARG has been declared, e.g. in order to override the
normal C<OUTPUT: RETVAL> behaviour, or to add a PERL_UNUSED_ARG(TARG) in
PPCODE for an XSUB which has a declared return value.
So the idea behind this commit is that targetable_legacy() will become
fossilised and will cause an early dXSTARG to continue being emitted
under the same circumstances; while targetable() will change over time,
allowing a late dXSTARG in more circumstances.
Currently, ExtUtils::Typemaps::OutputMap::targetable() is an object
method which examines the typemap template associated with the object
and determines whether it is suitable for the TARG optimisation. If so,
it returns a hash containing the parsed components of the
sv_setXv(...,...) string that were found in the typemap.
This commit changes this method in two ways.
First, targetable() now just returns a boolean, as the components are no
longer used.
Second, it converts targetable() into a class method, where the template
string is instead passed as a parameter.
This second change allows us to use the TARG optimisation even for
template code which wasn't derived from a typemap. In particular, for
the array() pseduo-type. For example in:
array(int,5)
foo(...)
CODE:
...
Formerly the 'array(int,5)' pseudo-type was converted into a template
like:
sv_setpvn($arg, (char*)$var, 5 * sizeof(int))
which was evalled and emitted as normal. But because that template
wasn't derived from a typemap, it wasn't eligible for TARG. Following
this commit, it is.
The code in ExtUtils::Typemaps::OutputMap->targetable() which looks
for typemap template code like
sv_setiv($arg, RETVAL);
sv_setpvn($arg, RETVAL, strlen(RETVAL));
is currently a bit rough and matches against various badly-formed
strings.
This commit changes it so that 2-arg functions like sv_setiv() are only
matched if they have 2 args, and sv_setpvn() only if it has three args.
Previously it would allow either to have 2 or 3 args.
This commit breaks a couple of recently-added tests for embedded
double-quoted strings because it turns out the regexes didn't actually
handle quotes, but matched anyway due to the laxness of the match.
This will be fixed shortly.
This commit is unlikely to affect any real XS code, because the code in
typemaps tends to be well-formed.
In the pattern-matching code which looks for candidates for the TARG
optinisation in typemap entries like:
sv_setiv($arg, RETVAL);
sv_setpvn($arg, RETVAL, strlen(RETVAL));
make C double-quoted strings such as "a,b" and "a,\",c" be parsed,
so that for example commas within double-quotes aren't mis-parsed
as an arg separator.
This commit is unlikely to affect any real XS code, but is added for
completeness.
It also changes a (??{ $bal }) to $bal_no_comma because
a) the third arg shouldn't have any commas;
b) there's no need for the (??{}) recursion yet - that's already
done within $bal/$bal_no_comma.
Previously, checking for whether a typemap code entry was suitable
for the TARG optimisation was done *before* eval-expanding the template;
so it would look for code like, e.g.
sv_setiv($arg, ...);
This commit changes it so that it now examines the code string *after*
expansion, so it now looks for strings like
sv_setiv(RETVALSV, ...);
The advantage of this is that a complex typemap entry such as
T_MYINT
${ "$var" eq "RETVAL" ? \"$arg = myint($var)"
: \"sv_setiv($arg, $var);" }
would now be a candidate.
Note that nothing in the current standard typemap, nor in any of the
XS code bundled with perl, actually benefits from this currently. So
it's more of a potential rather than an actual efficiency improvement.
Note also that ParseXS currently always expands $arg to 'RETVALSV', but
I've included a few other options to match, such as 'ST(N)', in case
that ever changes.
Currently sv_set(iv|uv|nv|pv|pvn) are candidates for using TARG for the return SV rather than creating a new mortal. In addition, sv_set(iv|uv|nv) are candidates for using TARG[iun] to more efficiently set the value of that TARG SV. This commit does two things. First, it extends the list of eligible functions to use TARG. I basically went through embed.fnc looking for functions of the form sv_set* and ignored the weird ones or ones which could leave TARG as anything other than a simple scalar (in particular, TARG mustn't become an RV or the referent will have its life extended to the life of the TARG). I added: - the _mg variants of the sv_set(iv|uv|nv|pv|pvn) functions. Since TARG shouldn't have set magic, it should make no difference whether we call set magic or not. - the 1-arg sv_set_(undef|false|true) functions. These relatively new functions don't take a parameter and so are unlikely to be used in a typemap, but are included for completeness. - sv_set_bool(). This is a relatively new function but might start to appear in typemaps in future. - sv_setpv_bufsize(). A bit obscure, but why not! Secondly, it extends the candidates for the use of TARG[iuv] to the _mg variants of sv_set[iun]v. Currently no typemaps or XS code in core uses these extra functions, but they might on CPAN or in the future.
The tail half of generate_output() looks like
if (cond) {
long block of code
}
else {
short block of code
}
return;
This commit refactors it to be
if (!cond) {
short block of code
return;
}
long block of code
return;
Next commit will reindent
Re-indent block after previous commit removed a scope. Whitespace-only.
Normally when an XSUB returns a *single* RETVAL value, we skip
emitting an EXTEND(SP,1), because the pp_entersub() will have earlier
been called with at least a GV or CV on the stack.
But it always extended the stack if there were any OUTLIST values that
needed pushing and returning. This commit makes it take into account
that a GV/CV plus at least min_args arguments were already on the stack
when pp_entersub() was called, so only extend the stack if more values
than that need returning.
So for example this no longer emits an EXTEND(SP,1)
void
foo(OUTLIST int i)
(This is unlikely to speed up much real-life code, as nobody seems to
actually use OUTLIST.)
The typemap file used for testing ExtUtils::ParseXS, t/typemap, hasn't (for many years) been updated when the typemap bundled with perl, lib/ExtUtils/typemap, gets updated. This commit resyncs them by overwriting the test file with the system file. In the long term they really aught to be unified in some fashion.
About 12 commits ago in this branch, I moved OUTLIST return-code
generation into the same code path as for that done for RETVAL.
This allows OUTLIST vars to potentially take advantage of some of the
optimisations done for RETVAL, such as using TARG rather than
sv_newmortal().
Unfortunately that commit missed a couple of places in the RETVAL code
path which had the variable name hard-coded as 'RETVAL'. This commit
fixes that by always using $var.
It would only have affected code which used an OUTLIST var of type SV*
on a non-void XSUB where the T_SV OUTPUT typemap entry had also been
overridden to:
T_SV
$arg = $var;
In that specific case, an XSUB like:
int
foo(OUTLIST SV* A)
was generating bad code like:
int RETVAL;
SV* A;
...
RETVAL = A;
RETVAL = sv_2mortal(RETVAL);
ST(1) = RETVAL;
It now emits:
int RETVAL;
SV* A;
...
A = sv_2mortal(A);
ST(1) = A;
This commit also adds a number of new tests for OUTLIST vars where the
typemap has an assign.
It also fixes the test framework in t/001-basic.t to capture and
display the error if the test code gives an eval error.
Do a general rewrite of all the code comments directly above and in this sub, to reflect all the recent refactoring work.
I recently added two fields to the ParseXS object,
xsub_targ_declared
xsub_targ_usable
and a few weeks later I found myself confused by them, so this commit
renames them and changes their functionality slightly.
xsub_targ_declared -> xsub_targ_declared_early
This flag now indicates solely that a dXSTARG was emitted at the *start*
of the XSUB in a wide scope. It no longer indicates also that a dXSTARG
been emitted later in a narrow scope.
xsub_targ_usable -> xsub_targ_used
Inverts the logic, and now indicates purely whether we've already
used TARG to return a value, rather than whether it's still available
to be used. That was getting muddied up with whether dXSTARG had been
emitted early (because if it hadn't, is TARG usable?).
Should be no functional change to the emitted C code.
Rename generate_output() to as_output_code() and make it a method in the ExtUtils::ParseXS::Node::Param class (it was formerly an ExtUtils::ParseXS method). As well as the rename, this commit mainly does s/$self/$pxs/g and s/$param/$self/g. The next commit will move this method into Node.pm.
The previous commit made as_output_code() be a method of
ExtUtils::ParseXS::Node::Param; now move it into Node.pm where it
belongs.
No functional changes; just a literal cut+paste, except for no longer
needing the full package name in the sub declaration, so
sub ExtUtils::ParseXS::Node::Param::as_output_code { ... }
becomes
sub as_output_code { ... }
It also updates the code comments for as_code() to add mention that
there's now also a related as_output_code() method.
Based on a PR review of this branch, fix a couple of typos and improve the description of when as_output_code() is called to handle an OUTLIST param (it's when $out_num is *defined*).
Earlier in this branch I added sv_setpv_bufsize() to the list of candidate functions which can use the TARG optimisation, but Tony C pointed out in code review that it doesn't make much sense, so I've removed it again.
It's annoying to keep incrementing the number of tests.
I introduced a bug during recent code refactoring. For the specific
(and unusual) case of a void XSUB with a single parameter which was
both OUTLIST and also had an OUTPUT entry with a code override:
foo(IN_OUTLIST int A)
OUTPUT:
A setA(ST[99], A);
the OUTLIST would fail to emit the code to set the new mortal to be
stored at ST(0) to the value of A.
iabyn
force-pushed
the
davem/xs_refactor9
branch
from
c6a18e7 to
22b2ca7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ParseXS: heavily refactor generate_output(), the function which emits the C which returns the value of an variable.
This series of approx 50(!) commits simplifies and reorganises that method so that it is much more regular, and with fewer special cases. For example, the code to handle 'OUTLIST var' now shares the same code path as RETVAL, so it can now also take advantage of the various optimisations which used to apply only to RETVAL, such as using TARG.
Many more tests have also been added.
Also, generate_output() has been renamed to as_output_code() and becomes a method of ExtUtils::ParseXS::Node::Param: part of the gradual shift towards having an AST and methods which act upon it.
There is still more work to be done in this area of code generation, but this branch is long enough already, so I will return to it later.