[otbn, rtl] Fix X-propagation issue in prefetch stage

[GregAC]

When executing near Xs in imem it's possible for Xs to be received on the incoming imem data. The prefetcher looks at the incoming data to see if its a branch instruction.

Previous to this change this branch check took priority over a check related to looping which altered the request going to imem. So when the incoming imem data was X, when simulation propagated Xs though ifs we end up with an X on the imem request address.

With this change the check for the branch happens regardless of whether or not the looping related logic activates and the branch logic itself does not alter the imem request so it avoids generating Xs on the external (to OTBN core) imem request interface.

Draft PR for as I don't think this is an essential fix. When running a regression I'm seeing some new failures though I think they were hidden behind previous X check failures (overall pass rate is similar). Further investigation should be done before merging this.

Fixes #21658 along with #22115

[rswarbrick]

I'm not sure I properly understand this change, with the exact if/else tree. Are we essentially just reversing the order of the last two cases in the if/else list? I think that would be easier to understand, so I fear I'm missing something...

[rswarbrick]

Nit: Indentation is a bit off here.

[GregAC]

Are we essentially just reversing the order of the last two cases in the if/else list?

Not quite. Previously the first (the insn_is_branch check) of those cases took priority over the second (the loop related one) now they sit inside one else as independent so both their if bodies can run or just one or neither.

[rswarbrick]

Oh, I think I understand now. Before this commit, the logic was something like:

if (!insn_fetch_req_valid_raw_i) A;
else if (insn_prefetch_fail) B;
else if (insn_is_branch(...)) C;
else if (xxxx) D;

With this commit, I think the structure becomes:

if (!insn_fetch_req_valid_raw_i) A;
else if (insn_prefetch_fail) B;
else {
    if (xxxx) D;
    if (insn_is_branch(...)) C;
}

I think the behaviour is now slightly different (even without Xs), because we might have a situation where both xxxx and insn_is_branch(...) are true. Before this patch, we'd get behaviour C. After this patch, we get C; D.

I guess this makes perfect sense so that we properly handle the situation where xxxx is true and insn_is_branch(...) is X.

But the code is getting a bit hard to understand! Can't we pull the stuff that controls imem_rvalid_kill_d and insn_prefetch into its own always_comb? That way, the fact that it won't mess up imem_addr_o becomes obvious.

[GregAC]

But the code is getting a bit hard to understand! Can't we pull the stuff that controls imem_rvalid_kill_d and insn_prefetch into its own always_comb? That way, the fact that it won't mess up imem_addr_o becomes obvious.

I did actually try this and it generated a large number of errors. I think under the first two cases of the big if, these ones:

opentitan/hw/ip/otbn/rtl/otbn_instruction_fetch.sv

Lines 329 to 336 in 72db492

	if (!insn_fetch_req_valid_raw_i) begin
	// Keep prefetching the same instruction when a new one isn't being requested. In this
	// scenario OTBN is stalled and will eventually want the prefetched instruction.
	imem_addr_o = insn_prefetch_addr;
	end else if (insn_prefetch_fail) begin
	// When prefetching has failed prefetch the requested address
	imem_addr_o = insn_fetch_req_addr_i;
	end else begin

There's at least some times you don't want to kill an incoming instruction immediately after a branch, though I didn't really investigate the details. So your new always_comb would need to repeat checking these conditions and only do the prefetch kill if they don't occur. So I'd rather have it all in one place so if you alter the top two conditions you don't need to to do it in two separate places.

[rswarbrick]

Oh, I see. In which case, never mind :-) Thanks for explaining carefully.

I just realised that the indentation is still awry and I don't want to accidentally leave a green tick lying around :-)

[andreaskurth]

Thanks for the careful explanation, @GregAC, and the discussion, @rswarbrick. LGTM!