[AVR] : PR49313, fix PR29524

Richard Henderson schrieb:
> On 06/15/2011 02:47 AM, Georg-Johann Lay wrote:
>> +#if defined (L_loop_ffsqi2)
>> +;; Helper for ffshi2, ffssi2
>> +;; r25:r24 = r26 + zero_extend16 (ffs8(r24))
>> +;; r24 must be != 0
>> +;; clobbers: r26
>> +DEFUN __loop_ffsqi2
> 
> Why does this function have "loop" in its name?  The actual
> implementation is surely irrelevant.

hmmm. I needed some global name that can be referenced from __ffshi2
resp. __ffssi2. The function in itself is not very helpful as stand
alone. You prefer some other naming for such global helpers?

>> +DEFUN __ffshi2
>> +    clr  r26
>> +    cpse r24, __zero_reg__
>> +1:  XJMP __loop_ffsqi2
>> +    ldi  r26, 8
>> +    or   r24, r25
> 
> It probably doesn't matter to execution speed, but why the
> OR here, when you know that r24 is 0?  Wouldn't the logic
> be clearer spelling this with MOV?

The following instruction is BRNE, a conditional branch.
MOV does not modify condition code. So OR is used. Alternative would
be EOR.  Or MOV+TST (note that TST Rx is sugar for AND Rx,Rx).

>> +#if defined (L_ctzsi2)
>> +;; count trailing zeros
>> +;; r25:r24 = ctz32 (r25:r22)
>> +;; ctz(0) = 32
> 
> Note that GCC does not define ctz(0).  It's explicitly undefined.
> Why are you forcing a particular value here?

Yes, you are right. Following patchlet ok?

Johann


	* config/avr/libgcc.S (__ctzsi2, __ctzhi2):
	Map zero to 255.



> r~

On 06/17/2011 02:20 AM, Georg-Johann Lay wrote:
> Richard Henderson schrieb:
>> On 06/15/2011 02:47 AM, Georg-Johann Lay wrote:
>>> +#if defined (L_loop_ffsqi2)
>>> +;; Helper for ffshi2, ffssi2
>>> +;; r25:r24 = r26 + zero_extend16 (ffs8(r24))
>>> +;; r24 must be != 0
>>> +;; clobbers: r26
>>> +DEFUN __loop_ffsqi2
>>
>> Why does this function have "loop" in its name?  The actual
>> implementation is surely irrelevant.
> 
> hmmm. I needed some global name that can be referenced from __ffshi2
> resp. __ffssi2. The function in itself is not very helpful as stand
> alone. You prefer some other naming for such global helpers?

__ffsqi_nz perhaps?

> The following instruction is BRNE, a conditional branch.

Oops, missed that, sorry.

> Yes, you are right. Following patchlet ok?
> 
> Johann
> 
> 
> 	* config/avr/libgcc.S (__ctzsi2, __ctzhi2):
> 	Map zero to 255.

You'd also delete the COUNT_LEADING_ZEROS_0 definition in longlong.h.


r~

Richard Henderson schrieb:
> On 06/17/2011 02:20 AM, Georg-Johann Lay wrote:
>> Richard Henderson schrieb:
>>> On 06/15/2011 02:47 AM, Georg-Johann Lay wrote:
>>
>> Yes, you are right. Following patchlet ok?
>>
>> Johann
>>
>> 	* config/avr/libgcc.S (__ctzsi2, __ctzhi2):
>> 	Map zero to 255.
> 
> You'd also delete the COUNT_LEADING_ZEROS_0 definition in longlong.h.
> 
> r~

__clzsi2(0) still returns 32 as it does not use ctz. So if
implementation of ctz affects validity of COUNT_LEADING_ZEROS_0 that's
bit confusing...

Johann