Fix cycleclock::Now for RISC-V and PPC

[luismarques]

Fixes the following issues with the implementation of cycleclock::Now:

• The RISC-V implementation wouldn't compile due to a typo;

• Both the PPC and RISC-V implementation's asm statements lacked the
  volatile keyword. This resulted in the repeated read of the counter's
  high part being optimized away, so overflow wasn't handled at all.
  Multiple counter reads could also be misoptimized, especially in LTO
  scenarios.

• Relied on the zero/sign-extension of inline asm operands, which isn't
  guaranteed to occur and differs between compilers, namely GCC and Clang.

The PowerPC64 implementation was improved to do a single 64-bit read of
the time-base counter.

The RISC-V implementation was improved to do the overflow handing in
assembly, since Clang would generate a branch, defeating the purpose
of the non-branching counter reading approach.

I dug into the history of the project and the API usage, and it's not quite
clear that branching would actually be problematic. For instance, the gperftools
repo inherited the same code and eventually changed the PPC implementation to a
branching one (before dropping the code completely). Still, for the considered
use case the chosen approach seems sensible (if more complex and error-prone to
implement), so I have kept non-branching implementations.

[LebedevRI]

@lenary @asb thoughts on RISC-V changes?

[LebedevRI]

Hmm, ok, after staring at snippets made from that function on godbolt
i have to agree that this appears to be sane in general.
Might be good to hear from RISC-V folk, but other than that LG.

[dmah42]

This seems overall decent to me, though I wouldn't mind seeing some ppc/risc-v benchmark output before and after the change. i don't have access to either platform. maybe you do @luismarques ?

[lenary]

@luismarques showed me why my contribution was both wrong (variable naming), and bad (GCC and clang can eliminate some of the assembly if you don't have it in a single ASM block), leading to miscompiles, as he has explained.

I haven't had a moment to check or benchmark this on a risc-v system, and don't have powerpc systems to check this on.

I agree there are almost certainly improvements needing to be made to the risc-v LLVM backend around tuning sequences like this to avoid branches so we can avoid the inline assembly here, but reasonably this change improves things for versions of clang that have already been released, and also for RISC-V gcc.

With the caveat that I haven't managed to compile and test this, I'm happy for it to be committed.

[luismarques]

@luismarques showed me why my contribution was both wrong (variable naming), and bad (GCC and clang can eliminate some of the assembly if you don't have it in a single ASM block), leading to miscompiles, as he has explained.

Well, no need to be so negative about your contribution :)
You based your implementation on the PPC one, so it was fairly reasonable to assume that the things you hadn't changed were OK to rely on. The broader issue is that the software ecosystem was still bootstrapping, so it was difficult to test the changes.

@dominichamon

This seems overall decent to me, though I wouldn't mind seeing some ppc/risc-v benchmark output before and after the change. i don't have access to either platform. maybe you do @luismarques ?

I have two QEMU VMs, 32- and a 64-bit RISC-V ones. But they are quite finicky, especially the 32-bit one. Just compiling software and their dependencies without running out of memory on the RV32 VM is extremely difficult. This code is going to be vended into the LLVM copy, so that might provide some testing. But my plan was to get this approved upstream before porting the changes to LLVM. Perhaps the manual inspection might suffice for now, unless somebody can test the PPC changes?

[asb]

I've not tested this, but at least by inspection it seems correct. LGTM.

[dmah42]

Thank you all!