Re: [PATCH 2/5] [AARCH64] Change IMP and PART over to integers from strings.

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 21/10/16 14:59, James Greenhalgh wrote:
> On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote:
>> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> Here is finally an updated (fixed) patch (I did not implement the two
>> implementer big.LITTLE support yet, that will be for a different patch
>> since I also fixed the part no not being unique as a separate patch.
>> Once I get a new enough kernel, I will also look into doing the
>> /sys/cpu/* style detection first.
>>
>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions
>> (and tested hacking the location of the read in file to see if it
>> works with big.LITTLE and other formats of /proc/cpuinfo).
> 
> I'm OK with this in principle, but it needs some polish for pedantic
> style comments...
> 
>> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are
>> integer constants.
>> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change
>> implementer_id to unsigned char.
>> Change part_no to unsigned int.
>> (AARCH64_BIG_LITTLE): New define.
>> (INVALID_IMP): New define.
>> (INVALID_CORE): New define.
>> (cpu_data): Change the last element's implementer_id and part_no to integers.
>> (valid_bL_string_p): Rewrite to ..
>> (valid_bL_core_p): this for integers instead of strings.
>> (parse_field): New function.
>> (contains_string_p): Rewrite to ...
>> (contains_core_p): this for integers and only for the part_no.
>> (host_detect_local_cpu): Rewrite handling of implementation and part
>> num to be integers;
>> simplifying the code.
> 
>> Index: config/aarch64/aarch64-cores.def
>> ===================================================================
>> --- config/aarch64/aarch64-cores.def	(revision 241200)
>> +++ config/aarch64/aarch64-cores.def	(working copy)
>> @@ -32,43 +32,46 @@
>>     FLAGS are the bitwise-or of the traits that apply to that core.
>>     This need not include flags implied by the architecture.
>>     COSTS is the name of the rtx_costs routine to use.
>> -   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it can
>> -   be found in /proc/cpuinfo.
>> +   IMP is the implementer ID of the CPU vendor.  On a GNU/Linux system it
>> +   can be found in /proc/cpuinfo. A partial list of implementer IDs is
>> +   given in the ARM Architecture Reference Manual ARMv8, for
>> -   in /proc/cpuinfo.  For big.LITTLE systems this should have the form at of
>> -   "<big core part number>.<LITTLE core part number>".  */
>> +   in /proc/cpuinfo.  For big.LITTLE systems this should use the macro AARCH64_BIG_LITTLE
>> +   where the big part number comes as the first arugment to the macro and little is the
>> +   second.  */
> 
> Needs rewrapped for 80 char width.
> 

I don't think it's a good idea to line wrap the def files, some of them
are processed with AWK during configure and having a complete entry per
line avoids potential matching problems.

R.

>>  
>> -static bool
>> -valid_bL_string_p (const char** core, const char* bL_string)
>> + static bool
>> +valid_bL_core_p (unsigned int *core, unsigned int bL_core)
> 
> Stray space before static.
> 
>>  {
>> -  return strstr (bL_string, core[0]) != NULL
>> -    && strstr (bL_string, core[1]) != NULL;
>> +  return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core
>> +         || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core;
>> +}
>> +
>> +/* Returns the integer that is after ':' for the field. */
>> +static unsigned parse_field (const char *field)
> 
> parse_field should be on a new line, FIELD should be capitalised in the
> explanatory comment.
> 
> OK with the appropriate changes to rectify these points.
> 
> Thanks,
> James
>