This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add Exynos to host detection
ClosedPublic

Authored by evandro on Dec 7 2017, 2:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

evandro created this revision.Dec 7 2017, 2:18 PM
zturner added a subscriber: zturner.Dec 7 2017, 2:27 PM
zturner added inline comments.
llvm/lib/Support/Host.cpp
222 ↗(On Diff #126044)

Can you initialize Exynos to some value here?

233–234 ↗(On Diff #126044)

How about

if (I.consume_front("CPU part"))
  I.ltrim("\t :").getAsInteger(16, Part);

As an example of why I prefer consume_front, it fixes what appears to be above in the preceding if statement (about CPU variant), where you've hardcoded 8 but actually meant 11.

236 ↗(On Diff #126044)

Can you move the declaration of Exynos down here and just say:

unsigned Exynos = (Variant << 12) + Part;

That also simultaneously addresses my previous comment about initialization.

evandro marked 3 inline comments as done.Dec 7 2017, 2:38 PM
evandro added inline comments.
llvm/lib/Support/Host.cpp
233–234 ↗(On Diff #126044)

Makes sense.

evandro marked 2 inline comments as done.Dec 7 2017, 2:43 PM
kristof.beyls accepted this revision.Dec 8 2017, 12:03 AM

LGTM modulo the one nitpick for which I'm not sure if it is needed.

llvm/lib/Support/Host.cpp
237–241 ↗(On Diff #126053)

I guess you may need to use LLVM_FALLTHROUGH here to silence a warning with some compilers?

This revision is now accepted and ready to land.Dec 8 2017, 12:03 AM
evandro marked an inline comment as done.Dec 8 2017, 10:17 AM
This revision was automatically updated to reflect the committed changes.