Re: [Patch] gcn: Add __builtin_gcn_{get_stack_limit,first_call_this_thread_p}

[Andrew Stubbs <ams@codesourcery.com>]

On 19/11/2022 10:46, Tobias Burnus wrote:
> On 18.11.22 18:49, Andrew Stubbs wrote:
>> On 18/11/2022 17:20, Tobias Burnus wrote:
>>
>> This looks wrong:
>>
>>> +    /* stackbase = (stack_segment_decr & 0x0000ffffffffffff)
>>> +            + stack_wave_offset);
>>> +       seg_size = dispatch_ptr->private_segment_size;
>>> +       stacklimit = stackbase + seg_size*64;
> (this should be '*seg_size' not 'seg_size' and the name should be 
> s/seg_size/seg_size_ptr/.)

Yes, looking again I think the comment is misleading, but the code has 
the MEM so the dereference is there.

>>> +       with segsize = dispatch_ptr + 6*sizeof(int16_t) + 
>>> 3*sizeof(int32_t);
>>> +       cf. struct hsa_kernel_dispatch_packet_s in the HSA doc. */
>>> +    rtx ptr;
>>> +    if (cfun->machine->args.reg[DISPATCH_PTR_ARG] >= 0
>>> +        && cfun->machine->args.reg[PRIVATE_SEGMENT_BUFFER_ARG] >= 0)
>>> +      {
>>> +        rtx size_rtx = gen_rtx_REG (DImode,
>>> + cfun->machine->args.reg[DISPATCH_PTR_ARG]);
>>> +        size_rtx = gen_rtx_MEM (DImode,
>>> +                    gen_rtx_PLUS (DImode, size_rtx,
>>> +                          GEN_INT (6*16 + 3*32)));
>>> +        size_rtx = gen_rtx_MULT (DImode, size_rtx, GEN_INT (64));
>>> +
> (Reading it, I think it should be '..._MEM(SImode,' and 
> '..._MULT(SImode' instead of DImode.)

Yes, I think you're right; the field is uint32.

> Admittedly, there is probably something not quite right as I see with 
> gfx908
> 
>    # of expected passes            27476
>    # of unexpected failures        317
> 
> where 317 FAIL comes from 88 testcase files.
> 
> That's not a a very high number but more than the usual fails, which 
> shows that
> something is not quite right.
> 
>   * * *
> 
> I am pretty sure that I missed something - but the question is what.
> I hope you can help me pinpoint the place where it goes wrong.

This might be it:

> +	    if (cfun->machine->args.reg[PRIVATE_SEGMENT_WAVE_OFFSET_ARG] >= 0)
> +	      {
> +		rtx off;
> +		off = gen_rtx_REG (SImode,
> +		      cfun->machine->args.reg[PRIVATE_SEGMENT_WAVE_OFFSET_ARG]);
> +		ptr = gen_rtx_PLUS (DImode, ptr, off);
> +	      }

I think "off" needs to be zero-extended before you can add the SImode to 
DImode (same for the segment size, of course).

Andrew