CycleClock: Add support for Alpha architecture #1753

thesamesam · 2024-02-07T04:13:08Z

This uses the native RPCC instruction, which comes with a caveat. The
PCC (Program Cycle Register) tick counter on Alpha is a 32-bit unsigned
integer, which means that it overflows every ~4-ish seconds, depending
on clock speed. Therefore this is only safe to use for small
measurements. This is the same implementation used in fftw.

dmah42 · 2024-02-07T09:40:37Z

src/cycleclock.h

+  // which means it overflows every ~4sec depending on CPU frequency.  This
+  // tick count will only be reliable for short intervals.  This implementation
+  // is the same as what fftw uses.
+  uint64_t cc;


two thoughts: if we know it's 32-bits why do we read into a 64-bit and mask?

also if it's only 4 seconds i'm not sure it's going to be very useful. is there no alternative?

Does gettimeofday() (or an alternative) work on that arch? There's precedent for using that.

two thoughts: if we know it's 32-bits why do we read into a 64-bit and mask?

also if it's only 4 seconds i'm not sure it's going to be very useful. is there no alternative?

Hi, I helped work on this. It's read into a 64-bit because the register is 64-bits wide, the lower 32 bits are the actual counter, while the high 32 bits are "operating system dependent". Linux does not do anything with them today.

Does gettimeofday() (or an alternative) work on that arch? There's precedent for using that.

It works, the problem is that clock precision is too low. You can see this for example in the python bindings:

In [2]: time.clock_getres(time.CLOCK_MONOTONIC) Out[2]: 0.000976563

This has been the cause of many a PR of mine, as on alpha two calls to gettimeofday() less than ~1ms apart will return the same timestamp.

That said, the rpcc technique seems to sufficient to pass the test suite. I could include both options and make it configurable at compile-time perhaps?

I don't think it makes sense to make this configurable.
It should just work right (by the definition of "right",
that is expected by the users of that architecture),
or not at all.

I don't think it makes sense to make this configurable. It should just work right (by the definition of "right", that is expected by the users of that architecture), or not at all.

Then rpcc is the way to go. There isn't any method of measuring cycle counts for arbitrary periods on the architecture, so this is the best compromise.

the test suite is optimised to run quickly so that's not surprising.

i don't know if we have real world users that have per iteration timings of >4s but it wouldn't surprise me if we did. i think until there's a 64-bit high precision timer on arch, gettimeofday is going to be the better option.

s/per-iteration/per-repetition/, which is much worse

LebedevRI · 2024-02-07T15:25:52Z

src/sysinfo.cc

@@ -550,7 +550,9 @@ int GetNumCPUs() {
            "CPU ID assignments in /proc/cpuinfo seem messed up."
            " This is usually caused by a bad BIOS.\n");
  }
-  return num_cpus;
+  // If we are unable to identify the number of CPUs, default to one rather
+  // than zero.


This shouldn't be a global fix-up.

Can you elaborate what you mean? It should continue to default to zero and there should be code to parse this nonstandard /proc/cpuinfo? Or something else?

Presumably no other architecture so far has needed this fix-up,
so if this fix-up is needed for alpha, it should not be
retroactively added to other architectures at the same time.

If /proc/cpuinfo is different there, then you certainly should actually parse it in an alpha-specific way.

Presumably no other architecture so far has needed this fix-up, so if this fix-up is needed for alpha, it should not be retroactively added to other architectures at the same time.

If /proc/cpuinfo is different there, then you certainly should actually parse it in an alpha-specific way.

I believe there should be some sort of error condition then, rather than returning zero as if it was a valid number of CPUs. The calling code does not seem to consider this an error condition, and just blindly keeps going. For example, when returning zero CPUs, later code subtracts 1 from this (resulting in -1), then tries to cast to a size_t and allocate a vector of beyond SIZE_MAX length. So, not being able to parse /proc/cpuinfo does not seem to have been considered as a valid error condition.

Would you be okay with having this throw a runtime error if /proc/cpuinfo is unable to be parsed?

Once again, does that happen on Alpha, or on an other architecture?

we don't support exceptions.

I just tested, and returning -1 results in the same issues as returning 0. Here is an example of what it looks like with -1:

Running /var/tmp/portage/dev-cpp/benchmark-1.8.3/work/benchmark-1.8.3_build/test/benchmark_test Run on (-1 X 1987.32 MHz CPU ) Load Average: 1.09, 1.04, 0.90 terminate called after throwing an instance of 'std::length_error' what(): cannot create std::vector larger than max_size() <end of output> Test time = 0.05 sec ---------------------------------------------------------- Test Failed.

I see that it's used immediately in CPUInfo constructor without error checking. I'll have to experiment and see what is a reasonable way to propagate such an error up.

FWIW, personally, i just wouldn't bother.

Then how about std::abort() (or, I guess the abort handler stuff)? It looks like there's just no good way for callers to handle errors here and there are no circumstances under which you can meaningfully work with zero CPUs.

yeah we could.. the CHECK macros we have get compiled out in release builds but maybe we should have one that doesn't get compiled out (or just CallAbortHandler directly).

LebedevRI · 2024-02-12T17:56:31Z

src/sysinfo.cc

@@ -510,10 +510,13 @@ int GetNumCPUs() {
  int max_id = -1;
  std::ifstream f("/proc/cpuinfo");
  if (!f.is_open()) {
-    std::cerr << "failed to open /proc/cpuinfo\n";
-    return -1;
+    PrintErrorAndDie("Failed to open /proc/cpuinfo");


While there, could you please cleanup rest of this function, Ctrl+F for stderr?

LebedevRI · 2024-02-12T23:38:28Z

(generally looks fine to me, thank you)

LebedevRI · 2024-02-13T19:02:03Z

src/sysinfo.cc

  if (GetSysctl("hw.ncpu", &num_cpu)) return num_cpu;
-  fprintf(stderr, "Err: %s\n", strerror(errno));
-  std::exit(EXIT_FAILURE);
+  PrintErrorAndDie("Err: ", strerror(errno));


This becoming too messy :)
Please split this PR into two:

The actual changes to support the architecture

All of the PrintErrorAndDie stuff

Sorry, a little confused, isn't that what you asked for above?

I guess i thought it would look nicer?
Concretely, i'm getting stuck on how asymmetrical this all is,
sometimes there is a check for ncpu=0, sometimes there is not.

My next comment would be to extract the if ncpu<1 abort logic
into a wrapper function.

I just don't think it makes sense to do all that cleanup in this PR.

OK, will dropping the last commit be sufficient for this PR then?

"sysinfo: support parsing /proc/cpuinfo on Alpha" also has some PrintErrorAndDie stuff.

IMO that is immediately and directly relevant to this PR. I spent a lot of time investigating and trying to understand why the tests were failing, looking for issues with the cycle timer, when it just turned out that failing to parse /proc/cpuinfo silently returns garbage data that results in all sorts of cryptic errors down the line later. I believe handling this in a sensible way is super important in order to make porting to this and other platforms a reasonable process.

Hey, i'm only talking about how to get this diff to the state where i'd be happy to merge it,
and splitting it into two separate chunks will do that.
We spent more time arguing about that than it would have taken to get both of those PR's merged.

(please send that second PR please :))

LebedevRI

Ship it!

As documented, the real cycle counter is unsafe to use here, because it is a 32-bit integer which wraps every ~4s. Use gettimeofday instead, which has a limitation of a low-precision real-time-clock (~1ms), but no wrapping. Passes test suite. Signed-off-by: Sam James <sam@gentoo.org>

And also, bail out if unable to parse /proc/cpuinfo. This will preemptively alert users on platforms that need custom code for parsing /proc/cpuinfo. Signed-off-by: Sam James <sam@gentoo.org>

Signed-off-by: Sam James <sam@gentoo.org>

LebedevRI · 2024-02-13T21:04:56Z

@thesamesam thank you!

thesamesam · 2024-02-13T23:00:23Z

Thank you all!

Defines a wrapper function, CheckNumCPUs, which enforces that GetNumCPUs never returns fewer than one CPU. There is no reasonable way to continue if we are unable to identify the number of CPUs. This is a followup to google#1753. Signed-off-by: Sam James <sam@gentoo.org>

Accepted upstream already. See: google/benchmark#1753 See: google/benchmark#1756 Bug: https://bugs.gentoo.org/922877 Signed-off-by: Matoro Mahri <matoro_gentoo@matoro.tk>

Accepted upstream already. See: google/benchmark#1753 See: google/benchmark#1756 Bug: https://bugs.gentoo.org/922877 Signed-off-by: Matoro Mahri <matoro_gentoo@matoro.tk> Closes: #35729 Signed-off-by: Sam James <sam@gentoo.org>

dmah42 reviewed Feb 7, 2024

View reviewed changes

LebedevRI reviewed Feb 7, 2024

View reviewed changes

thesamesam force-pushed the alpha branch from 9243903 to 85896e3 Compare February 12, 2024 16:20

LebedevRI reviewed Feb 12, 2024

View reviewed changes

thesamesam force-pushed the alpha branch from 85896e3 to 975fa24 Compare February 13, 2024 07:13

LebedevRI reviewed Feb 13, 2024

View reviewed changes

thesamesam force-pushed the alpha branch from 975fa24 to 90bb11a Compare February 13, 2024 20:48

LebedevRI approved these changes Feb 13, 2024

View reviewed changes

thesamesam added 3 commits February 13, 2024 23:57

sysinfo: support parsing /proc/cpuinfo on Alpha

b1bec2f

And also, bail out if unable to parse /proc/cpuinfo. This will preemptively alert users on platforms that need custom code for parsing /proc/cpuinfo. Signed-off-by: Sam James <sam@gentoo.org>

tabular_test: add a missing DoNotOptimize call

17f2f35

Signed-off-by: Sam James <sam@gentoo.org>

LebedevRI force-pushed the alpha branch from 90bb11a to 17f2f35 Compare February 13, 2024 20:57

LebedevRI merged commit 385033b into google:main Feb 13, 2024
80 checks passed

thesamesam deleted the alpha branch February 13, 2024 23:00

thesamesam mentioned this pull request Feb 14, 2024

sysinfo.cc: Always abort on GetNumCPUs failure #1756

Merged

matoro mentioned this pull request Mar 12, 2024

dev-cpp/benchmark: add patches for alpha architecture gentoo/gentoo#35729

Closed

dnhn mentioned this pull request May 23, 2024

google-benchmark 1.8.4 Homebrew/homebrew-core#172608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CycleClock: Add support for Alpha architecture #1753

CycleClock: Add support for Alpha architecture #1753

thesamesam commented Feb 7, 2024

dmah42 Feb 7, 2024

LebedevRI Feb 7, 2024

matoro Feb 7, 2024

LebedevRI Feb 7, 2024

matoro Feb 7, 2024

dmah42 Feb 8, 2024

LebedevRI Feb 8, 2024

LebedevRI Feb 7, 2024

matoro Feb 7, 2024

LebedevRI Feb 7, 2024

matoro Feb 7, 2024

LebedevRI Feb 7, 2024

dmah42 Feb 8, 2024

matoro Feb 8, 2024

LebedevRI Feb 8, 2024

matoro Feb 9, 2024

dmah42 Feb 9, 2024

LebedevRI Feb 12, 2024 •

edited

Loading

LebedevRI commented Feb 12, 2024

LebedevRI Feb 13, 2024

matoro Feb 13, 2024

LebedevRI Feb 13, 2024

matoro Feb 13, 2024

LebedevRI Feb 13, 2024

matoro Feb 13, 2024

LebedevRI Feb 13, 2024

LebedevRI Feb 13, 2024

LebedevRI left a comment

LebedevRI commented Feb 13, 2024

thesamesam commented Feb 13, 2024

CycleClock: Add support for Alpha architecture #1753

CycleClock: Add support for Alpha architecture #1753

Conversation

thesamesam commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

LebedevRI commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI left a comment

Choose a reason for hiding this comment

LebedevRI commented Feb 13, 2024

thesamesam commented Feb 13, 2024

LebedevRI Feb 12, 2024 •

edited

Loading