Re: [PATCH v3 29/34] tcg: Reorg function calls

[Ilya Leoshkevich]

On Thu, Dec 01, 2022 at 09:39:53PM -0800, Richard Henderson wrote:
> Pre-compute the function call layout for each helper at startup.
> Drop TCG_CALL_DUMMY_ARG, as we no longer need to leave gaps
> in the op->args[] array.  This allows several places to stop
> checking for NULL TCGTemp, to which TCG_CALL_DUMMY_ARG mapped.
> 
> For tcg_gen_callN, loop over the arguments once.  Allocate the TCGOp
> for the call early but delay emitting it, collecting arguments first.
> This allows the argument processing loop to emit code for extensions
> and have them sequenced before the call.
> 
> For tcg_reg_alloc_call, loop over the arguments in reverse order,
> which allows stack slots to be filled first naturally.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/helper-head.h |   2 +
>  include/tcg/tcg.h          |   5 +-
>  tcg/tcg-internal.h         |  22 +-
>  tcg/optimize.c             |   6 +-
>  tcg/tcg.c                  | 609 ++++++++++++++++++++++---------------
>  5 files changed, 394 insertions(+), 250 deletions(-)

...

> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index d08323db49..74f7491d73 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -547,7 +547,7 @@ void tcg_pool_reset(TCGContext *s)
>  
>  #include "exec/helper-proto.h"
>  
> -static const TCGHelperInfo all_helpers[] = {
> +static TCGHelperInfo all_helpers[] = {
>  #include "exec/helper-tcg.h"
>  };
>  static GHashTable *helper_table;
> @@ -565,6 +565,154 @@ static ffi_type * const typecode_to_ffi[8] = {
>  };
>  #endif
>  
> +typedef struct TCGCumulativeArgs {
> +    int arg_idx;                /* tcg_gen_callN args[] */
> +    int info_in_idx;            /* TCGHelperInfo in[] */
> +    int arg_slot;               /* regs+stack slot */
> +    int ref_slot;               /* stack slots for references */
> +} TCGCumulativeArgs;
> +
> +static void layout_arg_even(TCGCumulativeArgs *cum)
> +{
> +    cum->arg_slot += cum->arg_slot & 1;
> +}
> +
> +static void layout_arg_1(TCGCumulativeArgs *cum, TCGHelperInfo *info,
> +                         TCGCallArgumentKind kind)
> +{
> +    TCGCallArgumentLoc *loc = &info->in[cum->info_in_idx];
> +
> +    *loc = (TCGCallArgumentLoc){
> +        .kind = kind,
> +        .arg_idx = cum->arg_idx,
> +        .arg_slot = cum->arg_slot,
> +    };
> +    cum->info_in_idx++;
> +    cum->arg_slot++;
> +}
> +
> +static void layout_arg_normal_n(TCGCumulativeArgs *cum,
> +                                TCGHelperInfo *info, int n)
> +{
> +    TCGCallArgumentLoc *loc = &info->in[cum->info_in_idx];
> +
> +    for (int i = 0; i < n; ++i) {
> +        /* Layout all using the same arg_idx, adjusting the subindex. */
> +        loc[i] = (TCGCallArgumentLoc){
> +            .kind = TCG_CALL_ARG_NORMAL,
> +            .arg_idx = cum->arg_idx,
> +            .tmp_subindex = i,
> +            .arg_slot = cum->arg_slot + i,
> +        };
> +    }
> +    cum->info_in_idx += n;
> +    cum->arg_slot += n;
> +}
> +
> +static void init_call_layout(TCGHelperInfo *info)
> +{
> +    int max_reg_slots = ARRAY_SIZE(tcg_target_call_iarg_regs);
> +    int max_stk_slots = TCG_STATIC_CALL_ARGS_SIZE / sizeof(tcg_target_long);
> +    unsigned typemask = info->typemask;
> +    unsigned typecode;
> +    TCGCumulativeArgs cum = { };
> +
> +    /*
> +     * Parse and place any function return value.
> +     */
> +    typecode = typemask & 7;
> +    switch (typecode) {
> +    case dh_typecode_void:
> +        info->nr_out = 0;
> +        break;
> +    case dh_typecode_i32:
> +    case dh_typecode_s32:
> +    case dh_typecode_ptr:
> +        info->nr_out = 1;
> +        info->out_kind = TCG_CALL_RET_NORMAL;
> +        break;
> +    case dh_typecode_i64:
> +    case dh_typecode_s64:
> +        info->nr_out = 64 / TCG_TARGET_REG_BITS;
> +        info->out_kind = TCG_CALL_RET_NORMAL;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +    assert(info->nr_out <= ARRAY_SIZE(tcg_target_call_oarg_regs));
> +
> +    /*
> +     * Parse and place function arguments.
> +     */
> +    for (typemask >>= 3; typemask; typemask >>= 3, cum.arg_idx++) {
> +        TCGCallArgumentKind kind;
> +        TCGType type;
> +
> +        typecode = typemask & 7;
> +        switch (typecode) {
> +        case dh_typecode_i32:
> +        case dh_typecode_s32:
> +            type = TCG_TYPE_I32;
> +            break;
> +        case dh_typecode_i64:
> +        case dh_typecode_s64:
> +            type = TCG_TYPE_I64;
> +            break;
> +        case dh_typecode_ptr:
> +            type = TCG_TYPE_PTR;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +
> +        switch (type) {
> +        case TCG_TYPE_I32:
> +            switch (TCG_TARGET_CALL_ARG_I32) {
> +            case TCG_CALL_ARG_EVEN:
> +                layout_arg_even(&cum);
> +                /* fall through */
> +            case TCG_CALL_ARG_NORMAL:
> +                layout_arg_1(&cum, info, TCG_CALL_ARG_NORMAL);
> +                break;
> +            case TCG_CALL_ARG_EXTEND:
> +                kind = TCG_CALL_ARG_EXTEND_U + (typecode & 1);
> +                layout_arg_1(&cum, info, kind);
> +                break;
> +            default:
> +                qemu_build_not_reached();
> +            }
> +            break;
> +
> +        case TCG_TYPE_I64:
> +            switch (TCG_TARGET_CALL_ARG_I64) {
> +            case TCG_CALL_ARG_EVEN:

On a s390x host with gcc-11.0.1-0.3.1.ibm.fc34.s390x I get:

FAILED: libqemu-aarch64-softmmu.fa.p/tcg_tcg.c.o
../tcg/tcg.c: In function ‘init_call_layout’:
../tcg/tcg.c:739:13: error: case value ‘1’ not in enumerated type 
[-Werror=switch]
  739 |             case TCG_CALL_ARG_EVEN:
      |             ^~~~

The following helps:

--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -735,7 +735,7 @@ static void init_call_layout(TCGHelperInfo *info)
             break;
 
         case TCG_TYPE_I64:
-            switch (TCG_TARGET_CALL_ARG_I64) {
+            switch ((TCGCallArgumentKind)TCG_TARGET_CALL_ARG_I64) {
             case TCG_CALL_ARG_EVEN:
                 layout_arg_even(&cum);
                 /* fall through */

This looks like a gcc bug to me.
I still thought it would be worth mentioning here.

...

Best regards,
Ilya