Re: [HACKERS] [PATCH] Generic type subscripting

Hi,

On 2020-12-07 14:08:35 -0500, Tom Lane wrote:
> 1. I'm still wondering if TypeParamBool is the right thing to pass to
> LLVMFunctionType() to describe a function-returning-bool. It does
> seem to work on x64_64 and aarch64, for what that's worth.

> - v_ret = build_EvalXFunc(b, mod, "ExecEvalSubscriptingRef",
> - v_state, op);
> + param_types[0] = l_ptr(StructExprState);
> + param_types[1] = l_ptr(TypeSizeT);
> + param_types[2] = l_ptr(StructExprContext);
> +
> + v_functype = LLVMFunctionType(TypeParamBool,
> + param_types,
> + lengthof(param_types),
> + false);
> + v_func = l_ptr_const(op->d.sbsref_subscript.subscriptfunc,
> + l_ptr(v_functype));
> +
> + v_params[0] = v_state;
> + v_params[1] = l_ptr_const(op, l_ptr(TypeSizeT));
> + v_params[2] = v_econtext;
> + v_ret = LLVMBuildCall(b,
> + v_func,
> + v_params, lengthof(v_params), "");
> v_ret = LLVMBuildZExt(b, v_ret, TypeStorageBool, "");

The TypeParamBool stuff here is ok. Basically LLVM uses a '1bit' integer
to represent booleans in the IR. But when it comes to storing such a
value in memory, it uses 1 byte, for obvious reasons. Hence the two
types.

We infer it like this:

> /*
> * Clang represents stdbool.h style booleans that are returned by functions
> * differently (as i1) than stored ones (as i8). Therefore we do not just need
> * TypeBool (above), but also a way to determine the width of a returned
> * integer. This allows us to keep compatible with non-stdbool using
> * architectures.
> */
> extern bool FunctionReturningBool(void);
> bool
> FunctionReturningBool(void)
> {
> return false;
> }

so you should be good.

I think it'd be a better to rely on the backend's definition of
ExecEvalBoolSubroutine etc. For the functions implementing expression
steps I've found that far easier to work with over time (because you can
get LLVM to issue type mismatch errors when the signature changes,
instead of seeing compile failures).

I've attached a prototype conversion for two other such places. Which
immediately pointed to a bug. And one harmless issue (using a pointer to
size_t instead of ExprEvalOp* to represent the 'op' parameter), which
you promptly copied...

If I pushed a slightly cleaned up version of that, it should be fairly
easy to adapt your code to it, I think?

WRT the prototype, I think it may be worth removing most of the types
from llvmjit.h. Worth keeping the most common ones, but most aren't used
all the time so terseness doesn't matter that much, and
the llvm_pg_var_type() would suffice.

Greetings,

Andres Freund