This is an archive of the discontinued LLVM Phabricator instance.

AArch64: fix cpuinfo issues
ClosedPublic

Authored by pawosm01 on Dec 7 2017, 11:10 PM.

Details

Summary

AArch64: fix an issue with older /proc/cpuinfo layout

There are two /proc/cpuinfo layots in use for AArch64: old and new.
The old one has all 'processor : n' lines in one section, hence
checking for duplications does not make sense.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

pawosm01 created this revision.Dec 7 2017, 11:10 PM
fhahn added a subscriber: fhahn.Dec 8 2017, 2:49 AM
Hahnfeld requested changes to this revision.Dec 8 2017, 4:46 AM
Hahnfeld added a subscriber: Hahnfeld.

I already have https://reviews.llvm.org/D40357 for review which does the same (except skipping the duplicate cpuinfo). It is currently blocked by https://reviews.llvm.org/D40722, maybe you could take a look there, ARM should suffer from the same problem.

This revision now requires changes to proceed.Dec 8 2017, 4:46 AM
pawosm01 updated this revision to Diff 126279.Dec 9 2017, 12:24 PM
pawosm01 edited edge metadata.
pawosm01 edited the summary of this revision. (Show Details)
Hahnfeld added inline comments.Dec 9 2017, 4:34 PM
runtime/src/kmp_affinity.cpp
2032

Multiple questions here, could you maybe add some comments with the answers?

  1. This can only happen for the "old" format? And we can't diagnose duplicate field errors in the new format because we just assume that we are seeing the old format, right?
  2. The "old" format doesn't have other information than the "processor <id>" specified? Because the code further down relies on num_avail pointing to the current entry.
pawosm01 added a comment.EditedDec 10 2017, 3:29 PM

Example of old format layout (note that 'Processor :' and 'processor :' won't collide as we don't call strncasecmp()):

$ cat /proc/cpuinfo
Processor : Cortex A57 Processor rev 1 (aarch64)
processor : 0
processor : 1
processor : 2
processor : 3
Features : fp asimd aes pmull sha1 sha2 crc32
CPU implementer : 0x41
CPU architecture: AArch64
CPU variant : 0x1
CPU part : 0xd07
CPU revision : 1

Hardware : jetson_tx1
Revision : 0000
Serial : xxxxxxx
$

Brief analysis of the code (after my changes) showed it does the right thing for the example above. This pattern (two sections, "Processor" and "Hardware" with all the "processor" entries listed only in the first one) is consistent across different machines with old /proc/cpuinfo layout.

Hahnfeld accepted this revision.Dec 13 2017, 5:37 AM

LGTM if you add a comment in the code explaining why we need this (old format) and that this effectively disables error detection in the new format on AArch64

This revision is now accepted and ready to land.Dec 13 2017, 5:37 AM
This revision was automatically updated to reflect the committed changes.