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.

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

md5sum: make calloc() use calloc_beebs() and use XOR for result value #147

Merged
merged 2 commits into from
May 26, 2022

Conversation

hirooih
Copy link
Contributor

@hirooih hirooih commented Oct 16, 2021

Although malloc_beebs() was used for malloc(), but calloc_beebs() was not used for calloc().
As result calloc() failed.
Use calloc_beebs() for calloc() and increase the heap size for it.


The result value was the sum of four uint32_t values.
The result changes between 32bit and 64bit CPUs.

Using XOR (Exclusive OR) fixes the issue.

@jeremybennett
Copy link
Collaborator

@hirooih Thanks for this. We are explicit in our use or the beebs functions, we don't overload the standard functions, so users can see where we have made the changes. The correct fix is to change the call to calloc to be a call to calloc_beebs.

Can you explain more how the use of XOR makes the code more portable? We try to avoid rewriting code except to correct errors, in order to maintain a variety of coding styles in the benchmark.

BTW, the _beebs suffix is because may of the Embench benchmarks came originally from the Bristol-Embecosm Embedded Benchmark Suite (BEEBS).

Although malloc_beebs() was used for malloc(),
but calloc_beebs() was not used for calloc().

use calloc_beebs() for calloc() and increase the heap size for it.
The result value was the sum of four uint32_t values.
The result changes between 32bit and 64bit CPUs.

Using XOR (Exclusive OR) fixes the issue.
@Roger-Shepherd
Copy link
Contributor

I think there is a problem with the return line of benchmark_body. With my understanding of the promotion rules of C the expression (h0 + h1 + h2 + h3) will have type int as this is what is being returned from benchmark_body and on a 64-bit machine the hi (unsigned 32-bits) will be converted to ints and then added. There can be a problem in that the result can have more than 32-bits. As the operation is only being performed as a (crude) check the XOR solution would seem acceptable; it is not really part of the benchmark.

@hirooih
Copy link
Contributor Author

hirooih commented Oct 18, 2021

We are explicit in our use or the beebs functions, we don't overload the standard functions, so users can see where we have made the changes.

I see. I thought it was better not to change the original code.
I've pushed the fix.

Can you explain more how the use of XOR makes the code more portable?

benchmark_body() returns an int value. In md5sum it returns a sum of 4 uint32_t values. The expected value 0x3_0C0D_A225 is valid only on 64bit CPUs. XOR does not have the issue.

We try to avoid rewriting code except to correct errors, in order to maintain a variety of coding styles in the benchmark.

I think this is not the case. benchmark_body() is added by Embench.

@hirooih
Copy link
Contributor Author

hirooih commented Oct 18, 2021

@Roger-Shepherd
Thank you. You commented at the same time!

@jeremybennett
Copy link
Collaborator

@hirooih I'm sorry - I didn't get round to merging this earlier. Thanks for all your work.

@jeremybennett
Copy link
Collaborator

Now good to merge.

@jeremybennett jeremybennett merged commit 71efe00 into embench:master May 26, 2022
@hirooih
Copy link
Contributor Author

hirooih commented May 26, 2022

@jeremybennett

Thanks!

widlarizer pushed a commit to widlarizer/embench-iot that referenced this pull request Nov 29, 2023
Merge in INV/riscv-benchmark from htc/dev/etywoniak-add-embench to htc/master

* commit '9e5d0701c152a0c1860711d0871ba64f449f8903': (82 commits)
  Fix warnings
  Fix fake frequency, fix license header, make script inherit spike's ret code, fail on non-zero benchmark ret code
  Add spike cycle count parsing
  Move to directory
  Add spike RISC-V simulator support
  Add handling for multi-word C compilation commands like zig cc
  Update board.cfg
  Update link.ld
  Update link.ld
  Guard md5sum printf by DEBUG
  Jsonfix (embench#168)
  Adding Full Support to WallyVerilog (embench#162)
  fix md5sum RET parse error (embench#167)
  md5sum: make calloc() use calloc_beebs() and use XOR for result value (embench#147)
  RISC-V compilation example (embench#155)
  Reworded decription of the capabilities of x86 and M1 macs to run x86 and M1 binaries as I found the original confusing - and I wrote it. (embench#150)
  Tarfind: fix heap size to be multiple of the pointer size (embench#146)
  [TARFIND] Fix local scale factor, heap size, add iterations and add baseline values.
  [MD5SUM] Fix local scale factor, heap size, add iterations and add baseline values.
  Add tarfind benchmark
  ...
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.

3 participants