[PATCH] D94928: [llvm-mca] Add support for in-order CPUs

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:81
+
+      assert(ReadAdvance >= 0);
+      assert(WS->getCyclesLeft() != UNKNOWN_CYCLES);
----------------
asavonic wrote:
> asavonic wrote:
> > andreadb wrote:
> > > dmgreen wrote:
> > > > How come this asserts on ReadAdvance < 0? I though it was relatively common to have certain instructions requiring operands before the main stage the pipeline is based on.
> > > I agree with Dave here. This logic must also work for the case where ReadAdvance is a negative values. There is no reason why we should restrict this logic to cases where ReadAdvance is >=0. 
> > > 
> > > You also don't need the assert for UNKNOWN_CYCLES at line 82. If the number of "write cycles left" for a register is unknown, then you can safely ignore it, and leave `StallCycles` as is (i.e. simply `continue` to the next iteration of the loop).
> > > 
> > > If however the number of write cycles left is different than UNKNOWN_CYCLES, then you should always update `StallCycles` with the std::max between the actual `StallCycles` and `CyclesLeft - ReadAdvance`. A negative ReadAdvance would effectively "increase" the `CyclesLeft` (which is what we want to model here). 
> > > 
> > > For this to work, CyclesLeft must be manipulated as a signed quantity (note that UNKNOWN_CYCLES is also a signed quantity; its value is -1). std::max requires for operands to be of the same type, so this may require an extra cast for StallCycles.
> > > You should then get rid of the check at line 84, and always compute the std::max, to update StallCycles.
> > > 
> > > How come this asserts on ReadAdvance < 0? I though it was relatively common to have certain instructions requiring operands before the main stage the pipeline is based on.
> > 
> > I wasn't sure that it is a valid case. Thank you for the explanation. I've
> > adjusted the code to handle negative `ReadAdvance`. There seems to be no such cases for A55, so I added a test for M7.
> > 
> > 
> > You also don't need the assert for UNKNOWN_CYCLES at line 82. If the number of "write cycles left" for a register is unknown, then you can safely ignore it, and leave `StallCycles` as is (i.e. simply `continue` to the next iteration of the loop).
> 
> I'm not sure that we can just ignore it. If it is unknown, then it is not zero,
> so a register hazard should be detected. I've modified the code to return 1 cycle
> stall, so we can check that the value is "known" in the next cycle.
> 
> In any case, this should never happen, because `Instruction::execute` is called
> before a subsequent instruction issue, and it always sets `CyclesLeft` to a
> known value.
>  
> > If however the number of write cycles left is different than UNKNOWN_CYCLES, then you should always update `StallCycles` with the std::max between the actual `StallCycles` and `CyclesLeft - ReadAdvance`. A negative ReadAdvance would effectively "increase" the `CyclesLeft` (which is what we want to model here). 
> > 
> > For this to work, CyclesLeft must be manipulated as a signed quantity (note that UNKNOWN_CYCLES is also a signed quantity; its value is -1). std::max requires for operands to be of the same type, so this may require an extra cast for StallCycles.
> 
> Right, this is fixed now.
> 
> > You should then get rid of the check at line 84, and always compute the std::max, to update StallCycles.
> > 
> 
> The check is still needed to ensure that we don't cast a negative value to
> unsigned.
Sorry, I have only read your comment now.

It is true that, strictly speaking, UNKNOWN_CYCLES isn't equivalent to zero cycles.
However, in practice it shouldn't happen to have to deal with writes of UNKNOWN_CYCLES. 
If I remember correctly, for the out-of-order simulator, we optimistically ignore it.

If you really want to enforce at least a 1-cycle delay for it, then fair enough. However you shouldn't return immediately. Instead, you should simply update the stall cycles quantity and then `continue` to the next iteration of the loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94928