search

riscv-non-isa / riscv-trace-spec

RISC-V Processor Trace Specification
https://jira.riscv.org/browse/RVG-88
Creative Commons Attribution 4.0 International
165 stars 47 forks source link

Section 7.6.2 description #93

Open pdonahue-ventana opened 10 months ago

pdonahue-ventana commented 10 months ago

Section 7.6.2 says: "It can therefore continue reconstructing the execution path until it reaches the JALR, from where it can deduce that opcode B at looplabel is the final retired instruction." Why is looplabel the final retired instruction? I think that this might have been originally written for the specific situation in 7.5.1 but it wasn't updated to the more general 7.6.2 description.

IainCRobertson commented 10 months ago

Perhaps use of the word "final" here is causing confusion? The intended meaning is "the final retired instruction on the execution path up to the point reported in the format 1 or 2 packet containing the address of looplabel". I also don't see any relevance to section 7.5.1.

Can you clarify exactly what your concern is?

pdonahue-ventana commented 10 months ago

My assumption was that "the final retired instruction" means that this is the last/final instruction being traced, e.g. because trace was subsequently disabled. There are a few references elsewhere in the spec to "the final retired instruction in the block" which implies "final for now" rather than "final forever" (which is what I normally assume an unqualified "final" means).

I referred to 7.5.1 because that section is about (1) the final packets that come out when tracing ends and (2) the same type of case as 7.6.2. My assumption about "final forever" led me to assume that this sentence was talking about the 7.5.1 case where tracing ends.

This isn't a major problem. It just led to my brief confusion when I (re)read this fairly complicated description of these corner cases. The description is probably just going to be complicated by its very nature but I had to help someone else make sense of it all. If it's easy then I would propose some minor tweak to make it clear that it's "final for now" but if it involves lots of RVI approvals then I'm OK just letting it go.

IainCRobertson commented 10 months ago

Thanks Paul. I'm going to leave this Issue open as a reminder to rework for the next spec update. On it's own I don't think this is important enough to spin another update, especially as 2.0.1 as only just been published.

IainCRobertson commented 1 month ago

@pdonahue-ventana can you review https://github.com/riscv-non-isa/riscv-trace-spec/pull/157 to see if this addresses your concerns.