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

fix input values due to the change for signed values #105

Merged
merged 1 commit into from
May 21, 2019
Merged

Conversation

Kuree
Copy link
Collaborator

@Kuree Kuree commented May 21, 2019

image

Whenever the high bit for the input is high, i.e. when the value is larger than 0x80, the current fault will pad 1's to make an negative number. This patch use masks to mask them off.

That's the cause that why verilator failed in GarnetFlow but ncsim works: https://github.com/StanfordAHA/GarnetFlow/commits/master

EDIT:
Travis version is working with this fix: https://travis-ci.com/StanfordAHA/GarnetFlow/builds/112535673

@Kuree Kuree requested a review from leonardt May 21, 2019 03:05
@coveralls
Copy link

Pull Request Test Coverage Report for Build 930

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 76.382%

Totals Coverage Status
Change from base Build 928: 0.01%
Covered Lines: 1368
Relevant Lines: 1791

💛 - Coveralls

@leonardt
Copy link
Owner

Interesting, is this because without the mask, C is promoting the byte (char) to a 32 bit type (used by verilator?) which results in sign extension? Then adding the mask results in the promotion to 32 bits without extension because the mask numeric literal is treated as a 32 bit value?

@Kuree
Copy link
Collaborator Author

Kuree commented May 21, 2019

I believe so. char in C/C++ is signed by default. So there might be a sign extension during the implicit conversation. Bitwise & should correct this error regardless whether the input is signed or not.

@leonardt
Copy link
Owner

Confirmed via testing!

// promotion.c
#include "printf.h"


int main() {
    char byte = 0xEE;
    printf("%%hhx byte = %hhx\n", byte);
    printf("%%x   byte = %x\n", byte);
    printf("%%x   (int) byte = %x\n", (int) byte);
    printf("%%x   byte & 0xFF = %x\n", byte & 0xFF);
}

and

/tmp
❯ gcc promotion.c && ./a.out
%hhx byte = ee
%x   byte = ffffffee
%x   (int) byte = ffffffee
%x   byte & 0xFF = ee

Copy link
Owner

@leonardt leonardt left a comment

Choose a reason for hiding this comment

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

C char promotion strikes again!

@leonardt leonardt merged commit 434b944 into master May 21, 2019
@leonardt leonardt deleted the fix_io_size branch May 21, 2019 03:36
@leonardt
Copy link
Owner

Another thing we could try is using unsigned char instead of char for the input type of files. This design enforces that files are read as unsigned bytes. Are there cases where it makes sense to read file data as a signed byte ever?

@Kuree
Copy link
Collaborator Author

Kuree commented May 21, 2019

I need to confirm this with @jeffsetter and other people who do the application development. For DNN the inputs may be signed depends on how they process it.

@jeffsetter
Copy link

I was templating my testbench to support different types, including the signed and unsigned variants. Images support unsigned and signed types.

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.

4 participants