-
Notifications
You must be signed in to change notification settings - Fork 414
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.
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
[GSoC] Fix forVersusWhilePerf.chpl #6833
[GSoC] Fix forVersusWhilePerf.chpl #6833
Conversation
Sorry, didn't get a notification from the edit. Looking at this now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I'd like to see the whitespace comments I made resolved, but otherwise have no complaints.
I verified that a correctness and performance run for this test worked after the changes.
@@ -29,9 +32,19 @@ proc main() { | |||
else | |||
res2 = res2 / 2; | |||
j += 1; | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line adds some trailing whitespace
@@ -43,7 +56,7 @@ proc main() { | |||
writeln("For loop underwent ", numIterations, " iterations ", numTrials, | |||
" times in ", t1.elapsed(TimeUnits.milliseconds)/1000, " seconds"); | |||
writeln("While loop underwent ", numIterations, " iterations ", numTrials, | |||
" times in ", t2.elapsed(TimeUnits.milliseconds)/1000, " seconds"); | |||
" times in ", t2.elapsed(TimeUnits.milliseconds)/1000, " seconds"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the whitespace for the worse
Awesome, thanks! |
PR chapel-lang#6833 changed forVersusWhilePerf.c to be more comparable to the Chapel version. It changed some int's to int64_t's, but used the wrong printf formatter and didn't include stdint.h. Here I include stdint.h and cast the results in printf to long ints. I could have used the PRId64 formatter, but I never remember how portable that is, and we don't use it in any other tests, so I opted to just cast in order to quiet the regression for now.
Quiet forVersusWhilePerf regressions PR #6833 changed forVersusWhilePerf.c to be more comparable to the Chapel version. It changed some int's to int64_t's, but used the wrong printf formatter and didn't include stdint.h. This just includes stdint.h and cast the results in printf to long ints. I could have used the PRId64 formatter, but I never remember how portable that is, and we don't use it in any other tests, so I opted to just cast in order to quiet the regression for now.
Looking at current performance graphs one can notice that in case of LLVM chapel's version of this test there is slight performance degradation, after some investigation I realized that this test is not written well due to few factors:
After changing all "int" to "int64_t" and "static const int64_t" to "int64_t" the Chapel's version performance closely matches the C version performance.
I guess @lydia-duncan is the best person to review this PR since she wrote those tests in first place.