Re: [PATCH] tcg/optimize: Fix constant folding of setcond

[Richard Henderson]

On 12/6/24 03:58, wannacu wrote:
> The `z_mask` field of TCGTemp argument needs to be
> properly set for the upcoming `fold_setcond_zmask` call
>
> This patch resolves issues with running some x86_64
> applications (e.g., FontForge, Krita) on riscv64
>
> Signed-off-by: wannacu <wannacu2049@gmail.com>
> ---
>   tcg/optimize.c                   |  3 +++
>   tests/tcg/x86_64/Makefile.target |  1 +
>   tests/tcg/x86_64/setcond.c       | 28 ++++++++++++++++++++++++++++
>   3 files changed, 32 insertions(+)
>   create mode 100644 tests/tcg/x86_64/setcond.c
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index e9ef16b3c6..e580b8d8b1 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -834,6 +834,9 @@ static int do_constant_folding_cond1(OptContext *ctx, TCGOp 
> *op, TCGArg dest,
>                                ? INDEX_op_and_i32 : INDEX_op_and_i64);
>           TCGOp *op2 = tcg_op_insert_before(ctx->tcg, op, and_opc, 3);
>           TCGArg tmp = arg_new_temp(ctx);
> +        /* Set z_mask for the follwing `fold_setcond_zmask` call. */
> +        arg_info(tmp)->z_mask = (ctx->type == TCG_TYPE_I32
> +                                      ? UINT32_MAX : UINT64_MAX);
>   
>           op2->args[0] = tmp;
>           op2->args[1] = *p1;

The problem is stale data in tmp's arg_info, correct?  Better would be adding 
fold_and(op2) a few lines later, after finishing setup of op2's arguments.

> +++ b/tests/tcg/x86_64/setcond.c
> @@ -0,0 +1,28 @@
> +#include <stdint.h>
> +#include <assert.h>
> +
> +uint8_t test(uint8_t a)
> +{
> +    uint8_t res = 0xff;
> +    asm(
> +        "lea -0x1160(%%edi), %%edx\n\t"
> +        "lea -0xd7b0(%%edi), %%ecx\n\t"
> +        "cmp $0x9f, %%edx\n\t"
> +        "setbe %%dl\n\t"
> +        "cmp $0x4f, %%ecx\n\t"
> +        "setbe %%cl\n\t"
> +        "or %%ecx, %%edx\n\t"
> +        "cmp $0x200b, %%edi\n\t"
> +        "sete %0\n\t"
> +        : "=r"(res)
> +    );
> +    return res;
> +}
> +
> +int main(void)
> +{
> +    for (uint8_t a = 0; a < 0xff; a++) {
> +        assert(test(a) == 0);
> +    }
> +    return 0;
> +}

(1) The inline assembly is incorrect, and (2) the test does not fail without your patch. 
So it is difficult for me to tell exactly what you're trying to test.

Problems with the asm:
  - using edi directly instead of having a as an input,
  - passing uint8_t a, but using all 32-bits of edi; upper 24 are logically 
garbage.
  - not adding edx, ecx as clobbers, or better as temp arguments.
  - initializing res, but not adding as an input, or in-out argument.

I *think* you want

unsigned char test(unsigned a)
{
    unsigned char res = 0xff;
    unsigned t1, t2;

    asm("lea -0x1160(%3), %1\n\t"
        "lea -0xd7b0(%3), %2\n\t"
        "cmp $0x9f, %1\n\t"
        "setbe %b1\n\t"
        "cmp $0x4f, %2\n\t"
        "setbe %b2\n\t"
        "or %2, %1\n\t"
        "cmp $0x200b, %3\n\t"
        "sete %0\n\t"
        : "+r"(res), "=&r"(t1), "=&r"(t2) : "r"(a));
    return res;
}

But as the test doesn't ever fail for me, I can't tell.


r~