-
Notifications
You must be signed in to change notification settings - Fork 426
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
Instructions re-encoding aligned with stable pulp_encoding_blocks-OPHW-2022-11-14.xlsx #740
Conversation
…p_encoding_blocks-OPHW-2022-11-14 from issue openhwgroup#452. Changes wrt pulp_encoding_blocks-OPHW-jm-jpb-pgo-2022-04-26 : - new encoding for cv.extract, cv.insert, cv.shuffle, cv.pack, cv.cplxmul, cv.subrotmj, cv.add/subb.div2/4/8 - description fix for cv.insert, cv.clb, cv.bitrev, cv.avgu - added missing SIMD cv.sdotsp.sc.b Signed-off-by: Pascal Gouedo <[email protected]>
…straints). Signed-off-by: Pascal Gouedo <[email protected]>
Opting out of PULP related reviews |
Signed-off-by: Pascal Gouedo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this @pascalgouedo. I will leave it to @davideschiavone to approve as he is far more knowledgeable than me regarding the XPULP ISA.
docs/source/corev_hw_loop.rst
Outdated
|
||
- HWLoop body must contain at least 3 instructions. | ||
An illegal exception is raised otherwise. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when does this happen? when setting the END of the HW Loop? We should specify this in my opinion, as well as specify the ordering of settings, because if you check that the HW loop END instruction is greater or equal to START + 12 (or +16 actually as you point to the next after the end)- when setting the END of the loop, then START must be programmed before, otherwise, if you check when fetching the first time the first instruction of the HW Loop, then ordering does not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite complex ordering constraint.
A Loop Enable bit could solve the ordering problem when programming HW loops CSRs.
General answer could be what is stated in last conversation/comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry what is the last conversation/comment?
Anyway, for me it is fine any kind of solution, e.g. the easiest would be when fetching the first instruction of the HW Loop (start), but we need to say this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do nothing in HW for that, no exception.
docs/source/corev_hw_loop.rst
Outdated
|
||
- No Compressed instructions (RVC) allowed in the HWLoop body. | ||
An illegal exception is raised otherwise. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we say this happens when fetching the first time a compressed instruction inside the HW loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think any time because this is an unrecoverable error and program execution can not continue after that anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for me, shall we add some words to better specify it?
docs/source/corev_hw_loop.rst
Outdated
- No uncoditional jump instructions allowed in the HWLoop body. | ||
An illegal exception is raised otherwise. | ||
- No unconditional jump instructions allowed in the HWLoop body. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we say this happens when fetching the first time a jump instruction inside the HW loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as above.
docs/source/corev_hw_loop.rst
Outdated
- No coditional branch instructions allowed in the HWLoop body. | ||
An illegal exception is raised otherwise. | ||
- No conditional branch instructions allowed in the HWLoop body. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we say this happens when fetching the first time a branch instruction inside the HW loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as above.
docs/source/corev_hw_loop.rst
Outdated
|
||
- No privileged instructions (mret, dret, ecall, wfi) allowed in the HWLoop body, except for ebreak. | ||
An illegal exception is raised otherwise. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as above.
docs/source/corev_hw_loop.rst
Outdated
|
||
- No memory ordering instructions (fence, fence.i) allowed in the HWLoop body. | ||
An illegal exception is raised otherwise. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as above.
docs/source/corev_hw_loop.rst
Outdated
|
||
- The End address of the outermost HWLoop (#1) must be at least 2 | ||
instructions further than the End address innermost HWLoop (#0), | ||
i.e. HWLoop[1].endaddress >= HWLoop[0].endaddress + 8 | ||
An illegal exception is raised otherwise. | ||
An illegal instruction exception is raised otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the "at least 3 instructions", those 2 constraints are harder to tackle in hardware. It would require to add resources (adders/substractors) just to check these wrong cases. Not really nice to keep CV32E40P small and power efficient.
In my previous life, these cases were only managed by constraining compiler and were not checked by HW ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this, but I think you need these subtractors/adders anyway if you want to trigger an illegal exception.
We can share the adder by deciding when checking these features, for example, for "at least 3 instructions" we check in the first instruction of the hwloop, for "at least 2 instruction after..", at the end of the outermost loop, whatever the solution, we must provide an answer to the SW and Verification team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None required if no HW exception.
For SW, they should not generate those cases so no pb.
For Reference Model, emulate an instruction exception.
For verification, those cases should not be verified and so not generated. If it would happen anyway, there will be a difference for sure as Reference Model will generate an exception and the HW will not ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm I wouldn't go this direction, but I do not have strong opinions either.
We won't definitively sell it as "safe" then, but even if you do nothing in HW, the HW won't work, so we should write AT LEAST that the behaviour is undefined, and we should remove the statements that we raise an exception, as we won't - did I understand this correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this behaviour is not safe at all but E40P is not a safety/secure core.
And yes any undefined behaviour should be mentioned every time it happens.
For these 2 cases, even if the HW does not generate an exception, I would like to add a statement/section that Reference Model should still raise an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then you can say that the reference model does it, but I would not, I mean, in theory, the reference model should behave as the DUT, but I let @MikeOpenHWGroup chime in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I always say that the DUT should behave as the reference model. 😉
Seriously, this is an important topic. In the general case there are three conditions:
-
Illegal events that result in either unpredictable behavior and/or an unrecoverable state.
The testbench will be constrained to avoid generating these events. The reference model will not check for these events. -
Illegal events that cause an exception.
The testbench will be able to generate these events and will have functional coverage to ensure that it does. The reference model will also generate an exception. -
Illegal events that are automatically recoverable in the DUT.
The testbench will be able to generate these events and will have functional coverage to ensure that it does. The reference model will recover in the same way as the DUT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally agree with the first statement except when the reference model is not only used just for verification but also as an execution model for software guys when developing their application.
This is the case when this RM is embedded in a Virtual Platform.
If the RM is not generating an exception for case 1. then you are finding the issue only when simulating on RTL or FPGA.
If the compiler is not generating this case no pb but what if "someone" writes assembly code not respecting hw loops constraints?
I know I know writing assembly code !!!
Signed-off-by: Pascal Gouedo <[email protected]>
… loop body instructions number can be doubled by shifting it by 2 rather than 1. Signed-off-by: Pascal Gouedo <[email protected]>
Signed-off-by: Pascal Gouedo <[email protected]>
Signed-off-by: Pascal Gouedo <[email protected]>
Signed-off-by: Pascal Gouedo <[email protected]>
pulp_encoding_blocks-OPHW-2022-11-14.xlsx from issue #452.
Changes wrt pulp_encoding_blocks-OPHW-jm-jpb-pgo-2022-04-26.xlsx :