Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cycleclock: Fix type conversion to match function return type on riscv64 #1802

Merged
merged 1 commit into from
Jun 11, 2024
Merged

cycleclock: Fix type conversion to match function return type on riscv64 #1802

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cycleclock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ inline BENCHMARK_ALWAYS_INLINE int64_t Now() {
#else
uint64_t cycles;
asm volatile("rdtime %0" : "=r"(cycles));
return cycles;
return static_cast<int64_t>(cycles);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should check the range before the cast?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do that anywhere else though. I'm not sure what we would even do in that situation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert i guess? the C++ style guide talks about this a bit in discussing avoidance of unsigned types ("In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.")

but i agree we don't currently do it anywhere else. happy to punt this to another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://riscv.org/wp-content/uploads/2016/06/riscv-spec-v2.1.pdf, page 32:

The RDTIME pseudo-instruction reads the low XLEN bits of the time CSR, which counts wall-clock
real time that has passed from an arbitrary start time in the past. RDTIMEH is an RV32I-only instruction that reads bits 63–32 of the same real-time counter. The underlying 64-bit counter should
never overflow in practice.

At 10GHz clock rate, it would take ~30 years of uptime to overflow.

#endif
#elif defined(__e2k__) || defined(__elbrus__)
struct timeval tv;
Expand Down
Loading