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

Tue Mar 2 04:47:43 PST 2021

asavonic added a comment.

In D94928#2568386 <https://reviews.llvm.org/D94928#2568386>, @andreadb wrote:

> Now that https://reviews.llvm.org/D95954 has been fixed, there are only a few things left to do:
>
> 1. Disable the bottleneck analysis for in-order processors.
> 2. Add a couple of lines to the release notes describing this new feature.
> 3. Update the llvm-mca docs.

Thank you! Can you please check if wording in the docs is ok?

Regarding the bottleneck analysis: it seems to work for some cases
(A55-all-views.s), but not for others (A55-out-of-order-retire.s).
I will check what is wrong here. The feature is disabled for now as you
suggested.

================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:81
+
+      assert(ReadAdvance >= 0);
+      assert(WS->getCyclesLeft() != UNKNOWN_CYCLES);
----------------
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.

================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:81
+
+      assert(ReadAdvance >= 0);
+      assert(WS->getCyclesLeft() != UNKNOWN_CYCLES);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94928