Skip to content

Conversation

breyerml
Copy link

This PR fixes a bug introduced in #188 due to not adding the necessary <cstdint> headers when using inptr_t.

Additionally, as far as I understand the code, the current develop branch exhibits UB or, at best, implementation-defined behavior due to the new error checking introduced in #186. If I only enable, for example, the triad kernel, goldC is never written and, therefore, is equal to 0 (the value of startC in the Stream.h header). However, later, the error is calculate as

T eC = std::fabs(vC - goldC) / std::fabs(goldC);

, which results in a division by zero and sets eC to NaN. In the check function, eC is tested against NaN and reports a failing check, although vC is also zero, in which case the result should be marked as correct.
I added a (ugly) hotfix, but I'm currently unable to implement a better fix due to time constraints.

@breyerml
Copy link
Author

breyerml commented Mar 30, 2025

Also, if I run:

./omp-stream -s 10000000000 -n 2 --only Triad

all I get is a SegFault.
However, running it with

./omp-stream -s 1000000000 -n 2 --only Triad

gives to correct result.

(Note that our machine has enough RAM for the first call.)

@breyerml
Copy link
Author

Ok. The problem with this SegFault is that due to the read_arrays function, the STREAM data must be stored twice. Therefore, we actually have 6 * sizeof(T) * array_size instead of 3 * sizeof(T) * array_size, which is kind of unintuitive, to say the least.

@tomdeakin
Copy link
Contributor

tomdeakin commented Sep 5, 2025

Ok. The problem with this SegFault is that due to the read_arrays function, the STREAM data must be stored twice. Therefore, we actually have 6 * sizeof(T) * array_size instead of 3 * sizeof(T) * array_size, which is kind of unintuitive, to say the least.

This is a requirement because some models need to own their own data, either intrinsically or for performance reasons. Ideally we'd not need two copies, so would need to migrate all checking routines to each model. See #128

Copy link
Contributor

@tomdeakin tomdeakin left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand your comments, as goldC etc are initialised no matter what mode is run.

T goldC = startC;

OK, the problem is only with goldC because it is initialised to zero. I think we need a nicer solution to ensure that this is checked properly.

@tomdeakin
Copy link
Contributor

I'm not sure I understand your comments, as goldC etc are initialised no matter what mode is run.

T goldC = startC;

OK, the problem is only with goldC because it is initialised to zero. I think we need a nicer solution to ensure that this is checked properly.

Also fixed in #202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants