[MultiCycleDivider] Bug Fixes for Large Operands and Ready Indication Polarity #129
Conversation
| @@ -249,13 +252,7 @@ class MultiCycleDivider extends Module { | |||
| ~bBuf.getRange(0, dataWidth - 2).or() & | |||
| (signOut ^ signNum), | |||
| then: [ | |||
| // special logic for when a is also most negative # | |||
There was a problem hiding this comment.
Since you are in an If block, would you want to stay with If/Else and not insert a mux?
If you have the unit tests to exercise all paths through this logic, this is a quick refactor.
There was a problem hiding this comment.
That's fine with me. Since it's quick and non-functional here, I can add it to this PR.
| @@ -297,11 +300,11 @@ class MultiCycleDivider extends Module { | |||
| // conditionally convert negative inputs to positive | |||
| // and compute the output sign | |||
| aBuf < | |||
| mux(intf.dividend[dataWidth - 1] & intf.isSigned, | |||
There was a problem hiding this comment.
Optimization: these computations are using bit inversion and then adding 1 (e.g. 2s complement conversion).
First idea is: is there a way to stay in 1s complement as that +1 is very costly (full carry).
Second idea is: should we factor out a 2s complement converter so that this operation can be optimized and is very visible -- you don't have to use it, but it does make explicit this operation is not cheap.
There was a problem hiding this comment.
This might be a good one to discuss further and split out of this PR? Also note that given the way things are flopped, the 2's complement inversions shouldn't really cause timing problems (the longest path delays are pretty short by design). That being said, I'm all for optimization - just might make more sense to break it out as it's pretty unrelated to the change set here.
Description & Motivation
The MultiCycleDivider has the following bugs:
(1) Polarity of the readyIn output is flipped relative to what it should be.
(2) Unsigned cases of division involving very large operands (MSB=1) do not produce correct results and can even deadlock the unit.
This PR fixes these and adds tests for the exceptional cases moving forward.
Related Issue(s)
N/A
Testing
Added a new sequence for "evil" cases of division stressing large operands in both signed and unsigned. Fixed a bug in the testbench preventing testing of readyIn signal.
Backwards-compatibility
This change is backward compatible as the interface to the divider has not changed at all. Only the outputs for a given set of inputs have changed in certain cases.
Documentation
Documentation updated to address the case of signed division overflow which was also added as a test case and needs explicitly clarification on unit behavior.