Skip to content

Conversation

@richardleach
Copy link
Contributor

@richardleach richardleach commented Oct 31, 2025

my $x = (1,2,3); produces the following OP tree in blead:

2     <;> nextstate(main 1 -e:1) v:{ ->3
6     <1> padsv_store[$x:1,2] vKS/LVINTRO ->7
5        <@> list sKP ->6
3           <0> pushmark v ->4
-           <0> ex-const v ->-
-           <0> ex-const v ->4
4           <$> const(IV 3) s ->5
-        <0> ex-padsv sRM*/LVINTRO ->6

This is functionally equivalent to my $x = 3;:

2     <;> nextstate(main 1 -e:1) v:{ ->3
4     <1> padsv_store[$x:1,2] vKS/LVINTRO ->5
3        <$> const(IV 3) s ->4
-        <0> ex-padsv sRM*/LVINTRO ->4

Construction of the first tree typically generates "Useless use of X in scalar context" warnings, but special cases such as the constants 0 and 1 are excluded from these warnings.

This commit modifies the functions responsible for assigning scalar or void context to OPs to remove:

  • OP_NULL nodes with no kids and a following sibling.
  • OP_LIST nodes with only a single-scalar-pushing kid OP.

This transforms the first OP tree above into the second.

Besides having a "cleaner-looking" optree that's easier to follow when debuggging Perl code or porting, there are other practical benefits:

  • If the op_next chain hasn't been built, LINKLIST won't have to traverse these OP nodes and link them in. Subsequent compiler steps then won't re-traverse the same nodes to optimize them out of the op_next chain.
  • Anything traversing - or cloning - the full optree has fewer defunct OP nodes to visit.
  • OP slabs may contain a higher proportion of live OPs, reducing TLB pressure (on systems or workloads where that matters).

Note: this PR replaces #23523. It was easier to implement in op.c than I'd
originally anticipated and, as mentioned above, doing so reduces the number of
times such defunct nodes have to be processed during compilation.


  • This set of changes requires a perldelta entry and one is included.

`my $x = (1,2,3);` produces the following OP tree in blead:

    2     <;> nextstate(main 1 -e:1) v:{ ->3
    6     <1> padsv_store[$x:1,2] vKS/LVINTRO ->7
    5        <@> list sKP ->6
    3           <0> pushmark v ->4
    -           <0> ex-const v ->-
    -           <0> ex-const v ->4
    4           <$> const(IV 3) s ->5
    -        <0> ex-padsv sRM*/LVINTRO ->6

This is functionally equivalent to `my $x = 3;`:
    2     <;> nextstate(main 1 -e:1) v:{ ->3
    4     <1> padsv_store[$x:1,2] vKS/LVINTRO ->5
    3        <$> const(IV 3) s ->4
    -        <0> ex-padsv sRM*/LVINTRO ->4

Construction of the first tree typically generates "Useless use of X
in scalar context" warnings, but special cases such as the constants
`0` and `1` are excluded from these warnings.

This commit modifies the functions responsible for assigning scalar
or void context to OPs to remove:
* `OP_NULL` nodes with no kids and a following sibling.
* `OP_LIST` nodes with only a single-scalar-pushing kid OP.

This transforms the first OP tree above into the second.

Besides having a "cleaner-looking" optree that's easier to follow when
debuggging Perl code or porting, there are other practical benefits:
* If the op_next chain hasn't been built, LINKLIST won't have to traverse
these OP nodes and link them in. Subsequent compiler steps then won't
re-traverse the same nodes to optimize them out of the op_next chain.
* Anything traversing - or cloning - the full optree has fewer defunct
OP nodes to visit.
* OP slabs may contain a higher proportion of live OPs, reducing
TLB pressure (on systems or workloads where that matters).
@tonycoz
Copy link
Contributor

tonycoz commented Nov 3, 2025

The change looks reasonable, though doing a coverage test they aren't hit much, the first part:

  3307622: 2074:                    if (OP_TYPE_IS(o, OP_LIST) && !op_parent(o)) {
        -: 2075:                        /* Is the list now just an obvious scalar pushop?
        -: 2076:                         *     <@> list sKP ->6
        -: 2077:                         *         <0> pushmark v ->4
        -: 2078:                         *         <$> const(IV 3) s ->5
        -: 2079:                         */
      110: 2080:                        OP* first = cLISTOPo->op_first;
        -: 2081:                        assert(OP_TYPE_IS(first, OP_PUSHMARK));
     110*: 2082:                        OP* sib1 = OpSIBLING(first);
        -: 2083:                        assert(sib1);
      110: 2084:                        OP* sib2 = OpSIBLING(sib1);
      110: 2085:                        if (!sib2) {
       27: 2086:                            if (
       27: 2087:                                PL_opargs[sib1->op_type] & OA_RETSCALAR
        -: 2088:                            ){
        -: 2089:                                assert(sib1->op_next == sib1);
        -: 2090:                                /* Yup. The PUSHMARK and LIST are redundant.
        -: 2091:                                 * They can be stripped out. */
       27: 2092:                                op_sibling_splice(o,first,1,NULL);
       27: 2093:                                op_free(o);
       27: 2094:                                return sib1;
        -: 2095:                            }
        -: 2096:                        }
        -: 2097:                    }

The second part:

     1853: 2113:                        if (kid->op_next == kid || kid->op_next == sib) {
     1853: 2114:                            if (prev_kid->op_next == kid)
     1558: 2115:                                prev_kid->op_next = kid->op_next;
        -: 2116:
     1853: 2117:                            prev_kid->op_sibparent = kid->op_sibparent;
     1853: 2118:                            op_free(kid); kid = NULL;
        -: 2119:
        -: 2120:                            /* A NEXTSTATE with no sibling OPs is redundant
        -: 2121:                             * if another NEXTSTATE follows it. Null it out
        -: 2122:                             * rather than removing it, in case anything needs
        -: 2123:                             * to probe it for file/line/hints info. */
     1853: 2124:                            if (OP_TYPE_IS(prev_kid, OP_NEXTSTATE) && sib
     1549: 2125:                                && OP_TYPE_IS(sib, OP_NEXTSTATE)) {
        1: 2126:                                op_null(prev_kid);
        -: 2127:                            }
        -: 2128:                        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimise away LIST/ex-LIST trees containing one item

2 participants