Skip to content

Adding bignum in to the benchmarks. #40

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adding bignum in to the benchmarks. #40

wants to merge 3 commits into from

Conversation

Machiry
Copy link
Collaborator

@Machiry Machiry commented Aug 9, 2021

Added a new benchmark (bignum).
The workflow run corresponding to this: https://github.com/correctcomputation/actions/runs/3284592753?check_suite_focus=true

Copy link
Collaborator

@mattmccutchen-cci mattmccutchen-cci left a comment

Choose a reason for hiding this comment

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

Two issues with this benchmark that are not in the PR itself:

  1. I found some diffs between the tar file on gamera and the original tiny-bignum-c. Did you make these changes yourself, or where did they come from?

  2. In the workflow run, I noticed that the post-conversion build stopped after the failure in golden. In general, we want the post-conversion build to attempt all translation units and report all errors so that we get meaningful counts in summarize-benchmark-errors.py. I looked at the makefile, and it is weird: it has a single all rule that runs all the compilation commands in sequence, and it compiles bn.c multiple times, so warnings would be reported multiple times (if we ever track warnings in addition to errors). I'd like to replace the build rules with something saner; this will fix the error reporting and also allow the build to run in parallel for a slight speedup. Here's a draft of the new build rules I'm thinking of. Does this seem reasonable to you? If so, what would be the best way to incorporate the change into the workflow? Put a patch file in /home/github/checkedc-benchmarks on gamera and have the workflow apply it?

@@ -162,6 +162,15 @@ def is_allowed(self, var: Variant):
build_cmds=f'bear {make_checkedc}',
build_converted_cmd=f'{make_checkedc} -k'),

# bignum
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# bignum
# tiny-bignum-c

Can we please refer to this benchmark consistently as tiny-bignum-c (also in the PR title)? If you say "bignum", it's not obvious that you're talking about the same thing because there are many pieces of software with similar names on the web.

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