Re: [PATCH] Optimize manual byte swap implementations v2

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Feb 9, 2009 at 1:12 PM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> Hi Richard,
>
>> size is the number of significant bytes in n?  In this case it should
>> be a plain int.  As the above is a host dependence but relevant
>> for target code generation IMHO n should be a integer type of
>> the same width on all hosts (thus sth like int64_t).
>
> HOST_WIDEST_INT is 64 bit on almost every target I think.

HOST_WIDEST_INT is a host specific number.  For example
on i?86 it is 32bit by default, on x86_64 it is 64bit - so if you
build with -m32 on x86_64 you may get different optimization
results.  We should definitely avoid this host dependency, and
unconditionally using a 64 bit integer type would be better.

>>> +/* In order to limit the number of instructions considered while
>>> +   looking for a bswap implementation this number defines the maximum
>>> +   depth of the search.  */
>>> +static const int BSWAP_RECURSION_LIMIT = 8;
>>
>> How did you come up with this number?  Does it make sense to
>> make it a param?
>
> This is smallest number which makes the testcase run successful.  I'm not
> sure about making this a param. Essentially if someone comes up with a bswap
> implementation which is not recognized due to this value we should simply
> increase it.
> It might be worthwhile to have separate values for 32 and 64 bit?!

Ok, just leave it at hardcoded 8 for now.

>> whenever you use int_cst_value you have to check first if the
>> INTEGER_CST fits in a HOST_WIDE_INT.  So, replace all
>> of them with host_integerp (..., 1) and TREE_INT_CST_LOW.
>> Or, as the types will probably never be that large use
>> TREE_INT_CST_LOW unconditionally, which is a lot faster
>> here.
>
> I intend to only look for byte swaps if the target type is 32 or 64 bits in
> size and the only constants I'm looking at are operands of bit AND
> operations. So I think it is safe to assume that the constant alway fits
> into a HOST_WIDEST_INT - right?!

Yes, that's true.

>>
>>> +      (HOST_WIDE_INT)n->size * BITS_PER_UNIT)
>>> +    return false;
>>
>> Note that you assume that BITS_PER_UNIT is 8 elsewhere, this may
>> not be true!  So use 8 here as well.
>
> Yes I am forcing BITS_PER_UNIT to be 8 by only executing the pass if that's
> true nevertheless I would prefer BITS_PER_UNIT here instead of a magic
> number.

Hmhm.  Can you use a new BITS_PER_BYTE defined to 8 then? ;)

>> watch out for conversions in the num_ops == 2 case!  Which
>> kind of operations do you want to look at here?  There are
>> more elaborate predicates available than checking the number of ops.
>
> Ok. I'll replace these checks with
> gimple_assign_rhs_class (stmt) == GIMPLE_UNARY_RHS
> and
> gimple_assign_rhs_class (stmt) == GIMPLE_BINARY_RHS
>
>
> The new version of the patch is currently bootstrapping.

Thanks.
Richard.

> Bye,
>
> -Andreas-
>