Re: Minor codegen silliness in ExecInterpExpr()

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