Expand mcontrol6.hit into hit0 and hit1, and remove mcontrol6.timing.

[Tim Newsome (timsifive)]

This lets an implementation be more specific about when a trigger fired in relation to the instruction that matched, to avoid user confusion when triggers fire late.

It also simplifies the spec around desired trigger timing behavior, simply asking for best effort to match the recommendation from the table.

[Paul Donahue (pdonahue-ventana)]

I haven't thought through all the implications but here are some initial questions/comments:

1. How does this work with respect to action>1? For instance, if you have a load address trigger which turns trace on, do these bits get set based on whether the first traced instruction is the load itself, the instruction immediately after the load, or some asynchronous trace start event later?

2. If an implementation sometimes stops immediately after and sometimes later (e.g. depending on whether there are pipeline bubbles), is it legal for it to always call it the generic/vague "after" instead of trying to categorize as "more than one instruction after" vs. "the first instruction after"? It seems like it might be useful to loosen "more than one instruction after" to be "one or more" and then say that you should say "the first instruction after" if possible, but it's OK to say "one or more" if you want.

3. I think that this allows implementations that only support subsets like:

   • {false, before} (backwards compatible which is allowed because you say that hit1 can be 0)
   • {false, after} (definitely allowed because hit0 can be hardwired to 0)
   • {false, immediately after} (hit0 and hit1 read the same underlying bit) - is this allowed?
     That would be visible if somebody tried to write one of the other values to these WARL fields.

4. I think that "right before" and "immediately after" should be defined. *epc or dpc points to the instruction that hit the trigger or the next instruction after. There was previous verbiage that could be reused.

5. This loses the warning about load value triggers with timing=before and load side-effects. While debuggers would no longer need to be warned about asking for that, there still should be warnings so that hardware people can know that this is a bad idea and debuggers can know that they need to be careful when dealing with hardware that does this.

6. Are there any implications for chaining? It might actually make it easier but we should think this through.

This is a nontrivial change so I still want to think about it.

[YenHaoChen]

I like the idea of this PR. We generally prefer triggers fire before the instruction to give the user more insight, and we allow firing after for cases where the triggers cannot fire before. Additionally, the debugger can understand whether the instruction is executed by reading the tdata1 CSR.

I can provide a PR of hit0 and hit1 for Spike after approval of this PR.

(minor typo: "It they are implemented..." -> "If they are implemented...")

[Tim Newsome (timsifive)]

How does this work with respect to action>1? For instance, if you have a load address trigger which turns trace on, do these bits get set based on whether the first traced instruction is the load itself, the instruction immediately after the load, or some asynchronous trace start event later?

I would expect that in a given implementation the trigger fires when it fires, and the timing is suggested by the table. I suppose an implementation could fire at a different time depending on the action, but that seems like added complexity with no benefit.

If an implementation sometimes stops immediately after and sometimes later , is it legal for it to always call it the generic/vague "after" instead of trying to categorize as "more than one instruction after" vs. "the first instruction after"?

I would like implementation to be clear on whether the trigger hit immediately after, or later. That seems the most useful to a user. Otherwise everyone will just report the vague after and the distinction is lost. Is it onerous to have to report the distinction?

(hit0 and hit1 read the same underlying bit) - is this allowed? That would be visible if somebody tried to write one of the other values to these WARL fields.

hit0/hit1 are WARL, so I think that is allowed.

I think that "right before" and "immediately after" should be defined. *epc or dpc points to the instruction that hit the trigger or the next instruction after. There was previous verbiage that could be reused.

Done.

This loses the warning about load value triggers with timing=before and load side-effects.

It's back.

Are there any implications for chaining? It might actually make it easier but we should think this through.

I think it makes things a little more fuzzy, but ultimately probably simpler. Now it seems possible that someone decides to implement chains of instruction address before + load value after, which previously were prohibited (because timing didn't match).

What happens when a debugger sets a chain that can never fire because of hardware limitations? Or that can fire on some instructions but not on others?

[Tim Newsome (timsifive)]

Paul Donahue (@pdonahue-ventana) What do you need to feel good about this PR? More discussion in the mailing list/meeting? I want to get this going, since it affects some of e.g. the vector discussion.

[Paul Donahue (pdonahue-ventana)]

I'm OK with this in principle. Just wordsmithing.

[Paul Donahue (pdonahue-ventana)]

Perfect!

[Tim Newsome (timsifive)]

Perfect!

Not quite. I just pushed some more tweaks.

[Paul Donahue (pdonahue-ventana)]

The latest version still looks good.

[JamesKenneyImperas]

I haven't followed all the details here, but I'm wondering whether the same effect here can be achieved without introducing a backwards incompatibility with earlier mcontrol6.hit behavior.

The currently-specified behavior is:

hit0:0 hit1:0 -> not hit
hit0:1 hit1:0 -> hit before
hit0:1 hit1:1 -> hit immediately after
hit0:0 hit1:1 -> hit after, not immediately

I think that the before/after encoding here is redundant, because that is already reported by the timing field in the trigger. Therefore, an alternative implementation could be to preserve the original hit bit and rename the hit1 bit to imprecise (or something) which would get set only for a trigger with timing "after" that did not occur immediately. This would give these encodings:

hit:0 -> not hit
hit:1 timing:0 -> hit before
hit:1 timing:1 imprecise:0 -> hit immediately after
hit:1 timing:1 imprecise:1 -> hit after, not immediately

So the same information can be derived without backwards-incompatibility and with consistent meaning of "hit" in all implementations, whether or not they support the imprecise/hit1 bit.

Thanks.

[JamesKenneyImperas]

Ok, I think I see: the timing field is only a recommendation, not a requirement, so cannot be relied upon to determine actual timing.

[Tim Newsome (timsifive)]

Ok, I think I see: the timing field is only a recommendation, not a requirement, so cannot be relied upon to determine actual timing.

And also I hope to remove the timing bit in #807.