[PATCH] D98628: [MCA] Disable RCU for InOrderIssueStage

Mon Mar 22 09:36:42 PDT 2021

andreadb added inline comments.

================
Comment at: llvm/test/tools/llvm-mca/ARM/m7-negative-readadvance.s:63
+# CHECK-NEXT: [0,1]     .DE .   add.w	r1, r1, #2
+# CHECK-NEXT: [0,2]     . DeE   vldr	d0, [r1]

----------------
asavonic wrote:
> andreadb wrote:
> > I am assuming that this change is correct.
> > Not sure if we have already enough load/store tests. If not, then it may be worthy to add some tests (maybe as a follow-up. But only if you think that it is required).
> No, I think the change is incorrect. Thank you very much for spotting this!  VLDR is issued 1 cycle earlier, and it does not obey to -1 `ReadAdvance` anymore.
> 
> This happens because we check `ReadAdvance` only for current `WriteState` objects. Since we now retire instructions in the same cycle as they're executed, the corresponding `WriteState` objects are also invalidated in the same cycle. A subsequent instruction cannot check for a negative `ReadAdvance` because the `WriteState` objects are invalidated at this point.
> 
> I'm thinking on how to fix this. Can we free registers when an instruction is retired, but keep `WriteState` objects for N cycles more?
mm.. this is a bit of annoying to model as it would require a structural change.
I think that you can solve this problem by introducing a counter in the register file which acts as a timer. At the beginning of each cycle, that counter is incremented by one.
When a write is executed, we could store the actual value of that counter into one of its fields. So that we know the relative cycle at which it has finished execution. So even if the object is invalidated, we still retain the "last write cycle" information.

Not sure if it makes sense...

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98628/new/

https://reviews.llvm.org/D98628