Re: [PULL 11/24] tcg/optimize: Use tcg_constant_internal with constant folding

[Richard Henderson]

On 2/4/21 6:29 AM, David Hildenbrand wrote:
> On 04.02.21 17:04, Philippe Mathieu-Daudé wrote:
>> On 2/4/21 10:37 AM, David Hildenbrand wrote:
>>> On 04.02.21 10:29, Richard W.M. Jones wrote:
>>>>>> commit 8f17a975e60b773d7c366a81c0d9bbe304f30859
>>>>>> Author: Richard Henderson <richard.henderson@linaro.org>
>>>>>> Date:   Mon Mar 30 19:52:02 2020 -0700
>>>>>>
>>>>>>       tcg/optimize: Adjust TempOptInfo allocation
>>>>>>
>>>>>> The image boots just fine on s390x/TCG as well.
>>>>>
>>>>> Let me try this in a minute on my original test machine.
>>>>
>>>> I got the wrong end of the stick as David pointed out in the other email.
>>>>
>>>> However I did test things again this morning (all on s390 host), and
>>>> current head (1ed9228f63ea4b) fails same as before ("mount" command
>>>> fails).
>>>>
>>>> Also I downloaded:
>>>>
>>>>   
>>>> https://dl.fedoraproject.org/pub/fedora-secondary/releases/33/Cloud/s390x/images/Fedora-Cloud-Base-33-1.2.s390x.qcow2
>>>>
>>>>
>>>>
>>>> and booted it on 1ed9228f63ea4b using this command:
>>>>
>>>>     $ ~/d/qemu/build/s390x-softmmu/qemu-system-s390x -machine accel=tcg
>>>> -m 2048 -drive
>>>> file=Fedora-Cloud-Base-33-1.2.s390x.qcow2,format=qcow2,if=virtio
>>>> -serial stdio
>>>>
>>>> Lots of core dumps inside the guest, same as David saw.
>>>>
>>>> I then reset qemu back to 8f17a975e60b773d ("tcg/optimize: Adjust
>>>> TempOptInfo allocation"), rebuilt qemu, tested the same command and
>>>> cloud image, and that booted up much happier with no failures or core
>>>> dumps.
>>>>
>>>> Isn't it kind of weird that this would only affect an s390 host?  I
>>>> don't understand why the host would make a difference if we're doing
>>>> TCG.
>>>
>>> I assume an existing BUG in the s390x TCG backend ... which makes it
>>> harder to debug :)
>>
>> This seems to fix it:
>>
>> -- >8 --
>> diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc
>> index 8517e552323..32d03dbfbaf 100644
>> --- a/tcg/s390/tcg-target.c.inc
>> +++ b/tcg/s390/tcg-target.c.inc
>> @@ -1094,10 +1094,16 @@ static int tgen_cmp(TCGContext *s, TCGType type,
>> TCGCond c, TCGReg r1,
>>                   op = (is_unsigned ? RIL_CLFI : RIL_CFI);
>>                   tcg_out_insn_RIL(s, op, r1, c2);
>>                   goto exit;
>> -            } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) {
>> -                op = (is_unsigned ? RIL_CLGFI : RIL_CGFI);
>> -                tcg_out_insn_RIL(s, op, r1, c2);
>> -                goto exit;
>> +            } else if (is_unsigned) {
>> +                if (c2 == (uint32_t)c2) {
>> +                    tcg_out_insn_RIL(s, RIL_CLGFI, r1, c2);
>> +                    goto exit;
>> +                }
>> +            } else {
>> +                if (c2 == (int32_t)c2) {
>> +                    tcg_out_insn_RIL(s, RIL_CGFI, r1, c2);
>> +                    goto exit;
>> +                }
>>               }
>>           }
>> ---
>>
> 
> This works as well I think. Broken casting.
> 
> diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc
> index 8517e55232..f41ca02492 100644
> --- a/tcg/s390/tcg-target.c.inc
> +++ b/tcg/s390/tcg-target.c.inc
> @@ -1094,7 +1094,7 @@ static int tgen_cmp(TCGContext *s, TCGType type, TCGCond
> c, TCGReg r1,
>                  op = (is_unsigned ? RIL_CLFI : RIL_CFI);
>                  tcg_out_insn_RIL(s, op, r1, c2);
>                  goto exit;
> -            } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) {
> +            } else if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 :
> (TCGArg)(int32_t)c2)) {
>                  op = (is_unsigned ? RIL_CLGFI : RIL_CGFI);
> 
> (int32_t)c2 will be converted to (uint32_t) first, then to (TCGArg).
> Which is different than casting directly from (int32_t)c2 to (TCGArg).
> 
> Nasty.

Oh excellent.  Thanks for the find and analysis, guys.
I totally agree that this is a bug.

And it makes sense that extra constant propagation from the optimizer from the
patch in question might tickle this problem.



r~