[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);
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94928