Skip to content

Commit

Permalink
User Manual update : Added clipr/clipur note about rs2. (#944)
Browse files Browse the repository at this point in the history 
* Signed-off-by: Pascal Gouedo <[email protected]>

* Added clipr/clipur note about rs2.

Signed-off-by: Pascal Gouedo <[email protected]>

---------

Signed-off-by: Pascal Gouedo <[email protected]>
  • Loading branch information
pascalgouedo authored Feb 2, 2024
1 parent a227daa commit 1a58c7b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
6 changes: 6 additions & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,9 @@ TAGS
/build
/Bender.lock
/Bender.local
golden.src
revised.src
cadence_conformal
golden_reference_design
synopsys_formality
questa_autocheck
4 changes: 4 additions & 0 deletions docs/source/instruction_set_extensions.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -788,12 +788,16 @@ General ALU operations
| | else if rs1 >=rs2, rD = rs2, |
| | |
| | else rD = rs1 |
| | |
| | Note: rs2 is unsigned. |
+-------------------------------------------+------------------------------------------------------------------------+
| **cv.clipur rD, rs1, rs2** | if rs1 <= 0, rD = 0, |
| | |
| | else if rs1 >= rs2, rD = rs2, |
| | |
| | else rD = rs1 |
| | |
| | Note: rs2 is unsigned. |
+-------------------------------------------+------------------------------------------------------------------------+
| **cv.addN rD, rs1, rs2, Is3** | rD = (rs1 + rs2) >>> Is3 |
| | |
Expand Down

8 comments on commit 1a58c7b

@jstraus59
Copy link

Choose a reason for hiding this comment

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

Hi Pascal,

Thanks for posting this clarification.

FYI, I have reviewed the Imperas model implementation of cv.clipr and cv.clipur and I found it was not treating rs2 as unsigned. In particular, for cv.clipur when the MSB of rs2 is 1 then rD does not get clipped when it should (because a signed comparison is done where an unsigned comparison is needed.) I am curious whether this case is covered in the existing tests that compare against the Imperas reference model?

This also raises the question of the precise behavior of cv.clipr when the MSB of rs2 is set. In this case -(rs2+1) can exceed the range of negative numbers possible in a 32 bit 2's complement representation, so that calculation must use either saturating arithmetic or more than 32 bits when computing this value for comparison with rs1. Otherwise, unexpected results can occur if the 32 bit 2's complement value of -(rs2+1) wraps around.

Comparing a signed value with an unsigned value of the same number of bits is a bit tricky. I guess it is implied that the expressions in the specification are effectively infinite precision, but a comment to this effect might be helpful.

I have fixes for these issues in the next Imperas update.

Jim

@pascalgouedo
Copy link

Choose a reason for hiding this comment

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

Hi Jim,

I was looking to my mails backlog and saw an old one with this feedback. Sorry for the late answer.
I had exactly the same question beginning of the week from SW toolchain people so I analysed a bit HW behaviour.
And I saw that clipr/clipur results are totally wrong when rs2 MSB is 1.
So it comes to the conclusion that rs2 range is (0-0x7fffffff).

I will add this in clipr/clipur description in the User Manual.

Best regards,
Pascal.

@jstraus59
Copy link

@jstraus59 jstraus59 commented on 1a58c7b Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Any suggestions on what the ISS should do when this occurs. Options include

  1. Quietly implement the behavior based on arbitrary precision comparison (current behavior)
  2. Quietly implement the same behavior as the hardware, assuming that you could specify that
  3. Quietly set a fixed value for the result (perhaps 0 or -1)
  4. Nos. 1/2/3 + emit a warning message
  5. No. 4 but use the parameter that specifies the severity of the hwloop constraint checks to set the severity for the message, anything from ignore to fatal error

@MikeOpenHWGroup
Copy link
Member

Choose a reason for hiding this comment

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

  1. Nos. 1/2/3 + emit a warning message
  2. No. 4 but use the parameter that specifies the severity of the hwloop constraint checks to set the severity for the message, anything from ignore to fatal error

The warning and/or error message must be an invocation of uvm_warning or uvm_error(). My recommendation would be to emit a uvm_error(). We can always depreciate the error to a warning if so desired/needed. An example of that can be found in uvmt_cv32e40s_base_test_elaboration_workarounds.sv.

@Imperas
Copy link

Choose a reason for hiding this comment

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

uvm? My question was regarding the behavior of the iss, so uvm is not relevant.

But it does raise the issue of how this should be addressed when using the iss reference model in DV. Or should this case simply be avoided by DV tests?

@MikeOpenHWGroup
Copy link
Member

Choose a reason for hiding this comment

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

uvm? My question was regarding the behavior of the iss, so uvm is not relevant.

Haha! The only usage of the Imperas ISS at OpenHW is as a reference model for DV, so I sometimes forget it has other uses! So in our case, UVM is completely relevant.

should this case simply be avoided by DV tests?

It depends what @pascalgouedo puts in the User Manual. If he specifies that rs2[31] == 1 for cv.clipr rD, rs1, rs2 and cv.clipur rD, rs1, rs2 instructions yields unpredictable behaviour of the core, then yes, DV tests should avoid this. If he adds (something like) rs2[31] == 1 for cv.clipr rD, rs1, rs2 and cv.clipur rD, rs1, rs2 instructions results in an illegal instruction exception then we must test that.

@Imperas
Copy link

Choose a reason for hiding this comment

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

Yes exactly! Ideally this would be an illegal instruction exception which is very straightforward to model, test and verify.

However, a precedent has been set with the HW Loop "constraints" that the behavior is undefined when the constraints are violated but the hardware does no checking for them, presumably because hardware checks would take a lot of area or impact performance. But in addition, the documentation specifically states that an ISS should detect and issue error messages for HW Loop constraint violations.

This might be similar - if the decision is made both to not detect this in hardware AND to not specify the behavior, then perhaps the documentation should add a similar requirement for the ISS.

The Imperas ISS can do any of the options I listed above - but I would like some guidance added to the documentation, so I am not just making this <stuff> up.

@MikeOpenHWGroup
Copy link
Member

Choose a reason for hiding this comment

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

We are aligned on this @Imperas! Let's wait for the update to the User Manual and then modify the ISS to suit. No additional documentation required as by definition the RTL and RM must exhibit the same behaviour.

Please sign in to comment.