Re: [HACKERS] Minor codegen silliness in ExecInterpExpr() - Mailing list pgsql-hackers

From Andres Freund
Subject Re: [HACKERS] Minor codegen silliness in ExecInterpExpr()
Date
Msg-id 20170928221117.nczcjxhhxybslehb@alap3.anarazel.de
Whole thread Raw
In response to [HACKERS] Minor codegen silliness in ExecInterpExpr()  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: [HACKERS] Minor codegen silliness in ExecInterpExpr()
List pgsql-hackers
Hi,

On 2017-09-28 16:21:34 -0400, Tom Lane wrote:
> I noticed the following while poking around with perf:
>
>        |                             fcinfo->isnull = false;
>        |b5b:   movb   $0x0,0x1c(%rdx)
>        |                             *op->resvalue = op->d.func.fn_addr(fcinfo);
>   0.02 |       mov    0x8(%rbx),%rcx
>   1.19 |       mov    %rdx,%rdi
>   0.93 |       mov    %rdx,(%rsp)
>        |       mov    %rcx,0x8(%rsp)
>   0.01 |       callq  *0x28(%rbx)
>   2.17 |       mov    0x8(%rsp),%rcx
>        |       mov    %rax,(%rcx)
>        |                             *op->resnull = fcinfo->isnull;
>   1.18 |       mov    (%rsp),%rdx
>   4.32 |       mov    0x10(%rbx),%rax
>   0.06 |       movzbl 0x1c(%rdx),%edx
>   9.14 |       mov    %dl,(%rax)
>
> It looks to me like gcc believes it is required to evaluate "op->resvalue"
> before invoking the called function, just in case the function somehow has
> access to *op and modifies that.

Yea, the compiler probably doesn't have much choice besides assuming
that. Might be different if we'd annote function pointers as pure, and
used strict aliasing + perhaps some restrict markers, but ...


> We could save a pointless register spill
> and reload if there were a temporary variable in there, ie
>
>         EEO_CASE(EEOP_FUNCEXPR)
>         {
>             FunctionCallInfo fcinfo = op->d.func.fcinfo_data;
> +            Datum    fvalue;
>
>             fcinfo->isnull = false;
> -            *op->resvalue = op->d.func.fn_addr(fcinfo);
> +            fvalue = op->d.func.fn_addr(fcinfo);
> +            *op->resvalue = fvalue;
>             *op->resnull = fcinfo->isnull;
>
>             EEO_NEXT();
>         }
>
> and likewise in the other FUNCEXPR cases.
>
> This is on a rather old gcc, haven't checked on bleeding-edge versions.

Makes sense.  Do you want to make it so, or shall I?  I'd personally be
somewhat tempted to keep the branches in sync here...

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgsql-hackers by date:

Previous
From: Peter Geoghegan
Date:
Subject: Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple
Next
From: Robert Haas
Date:
Subject: Re: [HACKERS] The case for removing replacement selection sort