search

riscv-non-isa / tg-nexus-trace

RISC-V Nexus Trace TG documentation and reference code
https://jira.riscv.org/browse/RVG-96
Creative Commons Attribution 4.0 International
44 stars 32 forks source link

Feedback on chapter 9 - RISC-V N-Trace (Nexus-based Trace) Specification #25

Closed ved-rivos closed 8 months ago

ved-rivos commented 1 year ago

  1. Section 9.1 - "Both the constant load and the indirect unconditional jump must be traced as adjacent instructions (in same message/packet) in order for the decoder to be able to infer the indirect unconditional jump target."

    This statement should be reworded to avoid implying that the lui or auipc are traced in a message/packet. The term packet is also used here for first time and should be removed. Further, there is an implication that the decoder should be making this distinction. It should be the encoder that should use the combination of itype+sijump to make the decision to transform the itype from 8->9, 10->11, or 14->15. The decoder guidelines should be written to be clearer i.e. if the itype is a inferrable jump but the instruction at that pc in the binary is a indirect unconditional jump then disassemble forward (since disassembling backwards is not deterministic) to the branch and extract the constant from the instruction preceeding the branch.

  2. Section 9.1 - why should the section 9.1 not just reference section 3.1.1 of E-trace specification?

  3. Section 9.2 - The specification should drop the mode 1 and 2. They are not general and may not work for many programs e.g., C++ programs that do exception handling, programs tail calls, etc. Is there a reason why these should be retained as valid options?

  4. Section 9.3 - Reword "Long loops" to "Loops with many iterations such as those in functions like memcpy/strcpy have identical flow in each iteraction."

  5. Section 9.3 - The second paragraph describes a "simple loop" and third para a "long loop". To make it clearer the second paragraph should be reworded as "A typical loop is..." and where it says and "taken once at end" - prefix "typically" since this is a behavior of a predictable loop.

  6. Section 9.3 - suggest rewording this section to be more clearer. It will help if there were two subsections - one for BTM and one for HTM. Then in each subsection the text could be normatively written to explain what the rules are. Examples, should illustrate the rules and not be a means to determine the rules. It is also suggested that a formal language be used in the specification. This section is way too long to describe a very simple concept. Remove most of the discussion and simply provide the crisp specification.

  7. There is informative note about HREPEAT field overflow. This is the first time there is any discussion about HREPEAT field overflowing and it is not clear what such an overflow mean. Further the guidancec to generate several messages is not clear as to why on this condition several (how many?) messages should be generated with a max HREPEAT value.

  8. Is there a limit on HREPEAT value? Is it legal to use HREPEAT to be a 64 bit counter and so an infinite loop will never generate a resource full?

  9. Chapter 9 - first para - rephrase, not "must be"

  10. Section 9. 1 and 9.2 - These are specified in the ratified e-trace spec. Consider replacing with a reference to the E-trace spec to avoid repeating.

mipsrobert commented 8 months ago

I am closing all N-Trace PDF related issues with same comments as all issues were handled via comments/discussions in SINGLE Google Docs. Relevant links are as follows:

Notes to N-Trace PDF: https://docs.google.com/document/d/1h__c0Kc7TQAWMh5bw9cNC9bl_IGqyY_ylPV14uc2xj0

N-Trace PDF rc20: https://github.com/riscv-non-isa/tg-nexus-trace/commit/221f6b1cef94c5f503554e0cb0deb4046dd49686 N-Trace for ARC review: https://github.com/riscv-non-isa/tg-nexus-trace/commit/1de77dcff1a75197232f72b8fe6863892da0cc74