Re: [RFC/TileGX 1/6] fix args alignment bug

[Jiong Wang <jiwang@tilera.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030"; format=flowed, Size: 1907 bytes --]

ÓÚ 2013/1/18 23:00, Jiong Wang дµÀ:
> Hi Joel,
>
>   thanks for your careful review
>
>>> -  /* Loop backwards through arguments to determine stack 
>>> alignment.  */
>>> -  alignlen = 0;
>>> -
>>> -  for (j = nargs - 1; j >= i; j--)
>>> -    {
>>> -      typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>> -      alignlen += (typelen + 3) & (~3);
>>> -    }
>>> -
>>> -  if (alignlen & 0x4)
>>> -    stack_dest -= 4;
>> Do you have any hint as to why this code was written the way it was?
>> It doesn't seem like the type of code that would be added by accident.
>> Did Jeff (the apparent author) misunderstand the ABI?
>
>       actually, these these copied from our tilepro target which is a 
> pure 32bit target, and the
> compiler for that target is not gcc initially. so there maybe some 
> historic reason for those code.
>
>      but for tilegx, the ABI is actually very clear, 10 regs for 
> arguments,  then push on stack.
>
>      so I just delete them, gdb works ok on both daily usage and dejagnu
>>
>>>         typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
>>> -      slacklen = ((typelen + 3) & (~3)) - typelen;
>>> +      slacklen = ((typelen + 7) & (~7)) - typelen;
>> This is a detail, but can you use utils.c:align_up, in this case?
>> (I find those midly more readable than these incantations, but
>> it's OK if you prefer the code to stay as is).
>       thanks, I change to align_up to reuse existing helper function
>>
>>>     /* Add 2 words for linkage space to the stack.  */
>>> -  stack_dest = stack_dest - 8;
>>> -  write_memory (stack_dest, two_zero_words, 8);
>>> +  stack_dest = stack_dest - 16;
>>> +  write_memory (stack_dest, four_zero_words, 16);
>> It looks like you need to adjust the comment as well.
>
>    fixed.
>
>   is this new 01-revise.patch ok for this issue?
>
Ping, are this and the other updated tilegx patches OK?

Thanks,
Jiong