Re: WIP: expression evaluation improvements

Hi,

On 2019-10-24 14:59:21 -0700, Soumyadeep Chakraborty wrote:
> After looking at
> v2-0006-jit-Reference-functions-by-name-in-IOCOERCE-steps.patch, I was
> wondering
> about other places in the code where we have const pointers to functions
> outside
> LLVM's purview: specially EEOP_FUNCEXPR* for any function call expressions,
> EEOP_DISTINCT and EEOP_NULLIF which involve operator specific comparison
> function call invocations, deserialization and trans functions for
> aggregates
> etc. All of the above cases involve to some degree some server functions
> that
> can be inlined/optimized.

I don't think there's other cases like this, except when we don't have a
symbol name. In the normal course that's "just" EEOP_PARAM_CALLBACK
IIRC.

For EEOP_PARAM_CALLBACK one solution would be to not use a callback
specified by pointer, but instead use an SQL level function taking an
INTERNAL parameter (to avoid it being called via SQL).

There's also a related edge-case where are unable to figure out a symbol
name in llvm_function_reference(), and then resort to creating a global
variable pointing to the function. This is a somewhat rare case (IIRC
it's mostly if not solely around language PL handlers), so I don't think
it matters *too* much.

We probably should change that to not initialize the global, and instead
resolve the symbol during link time. As long as we generate a symbol
name that llvm_resolve_symbol() can somehow resolve, we'd be good. I
was a bit wary of doing syscache lookups from within
llvm_resolve_symbol(), otherwise we could just look look up the function
address from within there. So if we went this route I'd probably go for
a hashtable of additional symbol resolutions, which
llvm_resolve_symbol() would consult.

If indeed the only case this is being hit is language PL handlers, it
might be better to instead work out the symbol name for that handler -
we should be able to get that via pg_language.lanplcallfoid.

> If we do go down this road, the most immediate solution that comes to mind
> would
> be to populate referenced_functions[] with these. Also, we can replace all
> l_ptr_const() calls taking function addresses with calls to
> llvm_function_reference() (this is safe as it falls back to a l_pt_const()
> call). We could do the l_ptr_const() -> llvm_function_reference() even if we
> don't go down this road.

Which cases are you talking about here? Because I don't think there's
any others where would know a symbol name to add to referenced_functions
in the first place?

I'm also not quite clear what adding to referenced_functions would buy
us wrt constants. The benefit of adding a function there is that we get
the correct signature of the function, which makes it much harder to
accidentally screw up and call with the wrong signature. I don't think
there's any benefits around symbol names?

I do want to benefit from getting accurate signatures for patch
[PATCH v2 26/32] WIP: expression eval: relative pointer suppport
I had a number of cases where I passed the wrong parameters, and llvm
couldn't tell me...

Greetings,

Andres Freund