search

ucb-bar / chisel2-deprecated

chisel.eecs.berkeley.edu
387 stars 90 forks source link

Implement rsha_n correctly #576

Closed palmer-dabbelt closed 8 years ago

palmer-dabbelt commented 8 years ago

I'm actually not sure this is correct, but the other version is obviously wrong: it regularly shifts by an extremely large value (as the result of unsigned underflow). This actually manifests in rocket-chip, Howie found and isolated a test case.

I found this using "-fsanitize=undefined" (kind of by accident, since as far as I can tell this was just wrong), which seems like something that should be run on emulator.h. It's unfortunately a dynamic analysis, but we should probably run it on rocket-chip.

palmer-dabbelt commented 8 years ago

Someone who knows about numbers needs to look at this before merging it, I just guessed as to what the fix is (but it passed my test case, so I guess that's a good start).

da-steve101 commented 8 years ago

Could you add the exact case that was failing to the large number test suite as well in this pr?

da-steve101 commented 8 years ago

That looks correct to me, good catch. Added some comments to make it a bit more readable in a pull request to this branch #577

da-steve101 commented 8 years ago

perhaps one more thing, im not a fan of the redundant arguments. I think nw should be calculated inside the function from w

palmer-dabbelt commented 8 years ago

OK, there's a test in there now. It's not so great, because this particular failure only manifests itself when GCC optimizations are on, and they're not when running "make check".

While I agree it's kind of tricky to have the two arguments that could probably be calculated from each other, I don't really care to change it -- that's just a style issue, not a correctness issue, and I try not to get stuck dealing with style in Chisel.