This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add more CPUs to host detection
ClosedPublic

Authored by efriedma on Sep 11 2017, 5:02 PM.

Details

Summary

This returns "cortex-a73" for second-generation Kryo; not precisely correct, but close enough.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Sep 11 2017, 5:02 PM

Hi Eli,

Thanks for adding this.
I expect you to have a much better understanding of Kryo cores, so I can't add much comments on the specific constants you added in the code for those.
For Cortex-A73, the constants in this patch seem incorrect, looking at the Cortex-A73 TRM, see my detailed comments.

Thanks!

Kristof

lib/Support/Host.cpp
211 ↗(On Diff #114735)

The summary of this patch states returning cortex-a73 for second-generation Kryo, but this patch also introduces returning "kryo" for "0x211". Just checking if this was intentional.

unittests/Support/Host.cpp
108–110 ↗(On Diff #114735)

I don't have easy access to a Cortex-A73 system at the moment, but I'd expect the CPU implementer for a Cortex-A73 be 0x41, the same as for other Arm-designed cores.
Also, according to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.100048_0002_05_en/jfa1406793219362.html, the part number of Cortex-A73 is 0xD09, not 0x800.

efriedma added inline comments.Sep 12 2017, 11:51 AM
lib/Support/Host.cpp
211 ↗(On Diff #114735)

Yes, intentional; 0x211 is first-generation Kryo.

unittests/Support/Host.cpp
108–110 ↗(On Diff #114735)

Kryo second-generation is not a Cortex-A73. It's just close enough from the compiler's perspective that it isn't worth distinguishing here. (getHostCPUNameForARM already has separate detection code for actual A73 CPUs.)

kristof.beyls accepted this revision.Sep 12 2017, 12:12 PM
kristof.beyls added inline comments.
unittests/Support/Host.cpp
108–110 ↗(On Diff #114735)

ah - apologies, I didn't look at enough context code of this change.
I was confused by adding 2 "CPU parts" 0x800 and 0x801 both mapping to Cortex-A73, and didn't catch that both represent second-generation Kryo cores.
It now makes sense to me.

This revision is now accepted and ready to land.Sep 12 2017, 12:12 PM
This revision was automatically updated to reflect the committed changes.