[PATCH] D24661: More processors support under AARch64 state for auto detection.

[Suprateeka R Hegde via llvm-commits]

On 19-Sep-2016 02:13 PM, Renato Golin wrote:
> rengolin added inline comments.
>
> ================
> Comment at: lib/Support/Host.cpp:1122-1128
> @@ +1121,9 @@
> +  if (Implementer == "0x41") // ARM Ltd.
> +    // Look for the CPU part line.
> +    for (unsigned I = 0, E = Lines.size(); I != E; ++I)
> +      if (Lines[I].startswith("CPU part"))
> +        // The CPU part is a 3 digit hexadecimal number with a 0x prefix. The
> +        // values correspond to the "Part number" in the CP15/c0 register. The
> +        // contents are specified in the various processor manuals.
> +        return StringSwitch<const char *>(Lines[I].substr(8).ltrim("\t :"))
> +            .Case("0xd03", "cortex-a53")
> ----------------
> t.p.northover wrote:
>> I don't like how this scales with the many vendors. We'll end up with this loop duplicated half a dozen times before we're done.
>>
>> Perhaps convert the strings to numbers and use std::lower_bound on a static array?

I didnt understand this clearly.

Do you mean to say, the "for" loops are similar and repeated for every 
implementer (like ARM==0x41, APM==0x50, etc.)?

Hmm. Source code may look cluttered, but at runtime, it is only one 
"implementer" that goes through.

Or do you mean the "for" loop body itself is inefficient (and not the 
for loop being repeated) in the sense that there is lot of strcmp?

I agree, strcmp is bad and does not scale up. But I have the following 
thoughts:
1. Converting to numbers to be used in lower_bound on a static array 
needs maintenance (manual sorting). It might be prone to errors 
especially when the "CPU Part" strings (like 0xd03, etc.) change the 
sequence in future. One has to be very careful then.
2. Definitely reduces readability. The current approach is very neat.
3. Applicable only to IP-Cores licensing model (like ARM) and not for 
all processors in general.
4. The list may not really grow up to many such entries under one single 
implementer. In my opinion we can live with the penalty given other 
benefits.

In fact this whole thing of parsing /proc/cpuinfo itself is inefficient, 
but cant help in case of ARM. Even querying their Main ID Register 
(MIDR) or HWCAP is not complete. I just wish we had an system call 
interface (through libc) to query different aspects of procfs (/proc) - 
Some sort of a software-cpuid.

(WIN32 when they also support ARMv8-A, this would again be different)

Let me know your thoughts.

> Could we maybe add this to the target parser?

I would say, the placement is correct. No need to move to TargetPasrer. 
In my opinion, TargetParser, as the name suggests should consist of 
parsing the "target" (say, when -mcpu=vulcan is passed while building on 
a cortex-a53 based machine). Host.cpp is where -mcpu=native or such 
auto-detection of the host (or its attributes) should reside.

--
Supra