[opus] [Aarch64 v2 02/18] Reorganize ARM CPU #ifdefs.

[Jonathan Lennox]

> On Dec 8, 2015, at 12:13 PM, Timothy B. Terriberry <tterribe at xiph.org> wrote:
> 
> Jonathan Lennox wrote:
>> -# if defined(FIXED_POINT)
>> +# if defined(FIXED_POINT) && \
>> +	((defined(OPUS_ARM_MAY_HAVE_NEON) && !defined(OPUS_ARM_PRESUME_NEON)) || \
>> +		(defined(OPUS_ARM_MAY_HAVE_MEDIA) && !defined(OPUS_ARM_PRESUME_MEDIA)) || \
>> +		(defined(OPUS_ARM_MAY_HAVE_EDSP) && !defined(OPUS_ARM_PRESUME_EDSP)))
>>  opus_val32 (*const CELT_PITCH_XCORR_IMPL[OPUS_ARCHMASK+1])(const opus_val16 *,
>>      const opus_val16 *, opus_val32 *, int , int) = {
> 
> Maybe I'm missing something, but...
> 
>> -/*Is run-time CPU detection enabled on this platform?*/
>> -# if defined(OPUS_HAVE_RTCD) && (defined(OPUS_ARM_ASM) \
>> -   || (defined(OPUS_ARM_MAY_HAVE_NEON_INTR) \
>> -   && !defined(OPUS_ARM_PRESUME_NEON_INTR)))
>> +# if defined(OPUS_HAVE_RTCD) && \
>> +	(defined(FIXED_POINT) && \
>> +		((defined(OPUS_ARM_MAY_HAVE_NEON) && !defined(OPUS_ARM_PRESUME_NEON)) || \
>> +			(defined(OPUS_ARM_MAY_HAVE_MEDIA) && !defined(OPUS_ARM_PRESUME_MEDIA)) || \
>> +			(defined(OPUS_ARM_MAY_HAVE_EDSP) && !defined(OPUS_ARM_PRESUME_EDSP)))) || \
>> +	(!defined(FIXED_POINT) && \
>> +		(defined(OPUS_ARM_MAY_HAVE_NEON_INTR) && !defined(OPUS_ARM_PRESUME_NEON_INTR)))
>>  extern
>>  #  if defined(FIXED_POINT)
>>  opus_val32
>>  #  else
>>  void
>>  #  endif
>>  (*const CELT_PITCH_XCORR_IMPL[OPUS_ARCHMASK+1])(const opus_val16 *,
>>        const opus_val16 *, opus_val32 *, int, int);
> 
> Shouldn't the first case have a corresponding update in the #else clause 
> for the !defined(FIXED_POINT) case?

Hm, yes, you’re right — there are cases where we’ll end up emitting a (useless and unused, but harmless) CELT_PITCH_XCORR_IMPL object unnecessarily.  I’ll submit an updated version of the patch.

> 
>>    celt_pitch_xcorr_c,               /* ARMv4 */
>> diff --git a/celt/arm/pitch_arm.h b/celt/arm/pitch_arm.h
>> index eaf61c9..bd41774 100644
>> --- a/celt/arm/pitch_arm.h
>> +++ b/celt/arm/pitch_arm.h
>> @@ -46,7 +46,13 @@ opus_val32 celt_pitch_xcorr_edsp(const opus_val16 *_x, const opus_val16 *_y,
>>      opus_val32 *xcorr, int len, int max_pitch);
>>  #  endif
>> 
>> -#  if !defined(OPUS_HAVE_RTCD)
>> +#  if (defined(OPUS_ARM_MAY_HAVE_NEON) || \
>> +		defined(OPUS_ARM_MAY_HAVE_MEDIA) || \
>> +		defined(OPUS_ARM_MAY_HAVE_EDSP)) && \
>> +	(!defined(OPUS_HAVE_RTCD) || \
>> +		defined(OPUS_ARM_PRESUME_NEON) || \
>> +		(defined(OPUS_ARM_PRESUME_MEDIA) && !defined(OPUS_ARM_MAY_HAVE_NEON)) || \
>> +		(defined(OPUS_ARM_PRESUME_EDSP) && !defined(OPUS_ARM_MAY_HAVE_NEON) && !defined(OPUS_ARM_MAY_HAVE_MEDIA)))
>>  #   define OVERRIDE_PITCH_XCORR (1)
>>  #   define celt_pitch_xcorr(_x, _y, xcorr, len, max_pitch, arch) \
>>    ((void)(arch),PRESUME_NEON(celt_pitch_xcorr)(_x, _y, xcorr, len, max_pitch))
>> @@ -66,10 +72,13 @@ void celt_pitch_xcorr_float_neon(const opus_val16 *_x, const opus_val16 *_y,
>> 
>>  #endif /* end !FIXED_POINT */
>> 
> 
> And shouldn't this be the inverse of the previous two?

I think it’s logically equivalent (under the assumption that the MAY_HAVE and PRESUME defines are ordered), but the different representation is confusing.  I’ll align them.