[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:83
+        // Try again in the next cycle
+        return 1;
+      }
----------------
asavonic wrote:
> andreadb wrote:
> > asavonic wrote:
> > > andreadb wrote:
> > > > As I wrote before, you should simply ignore the case where the number of cycles left is equal to UNKNOWN_CYCLES.
> > > > You shouldn't early exit with an arbitrary number of cycles.
> > > > 
> > > > Basically what I am saying is that you should replace this return statement with a `continue;`
> > > > As I wrote before, you should simply ignore the case where the number of cycles left is equal to UNKNOWN_CYCLES.
> > > > You shouldn't early exit with an arbitrary number of cycles.
> > > > 
> > > > Basically what I am saying is that you should replace this return statement with a `continue;`
> > > 
> > > I mentioned this above, but I might be missing something.
> > > 
> > > What exactly UNKNOWN_CYCLES means in this case? I assume that for in-order
> > > pipeline it means that an instruction is issued (since we issue in-order), but
> > > we don't know when its write is going to be completed. If this is correct, then
> > > we should not issue any instructions that depend on this write until we know that the write
> > > is completed (and has CyclesLeft == 0).
> > > 
> > > `return 1` here stalls the instruction for 1 cycle, and it will be checked again
> > > in the next cycle. If we `continue` here instead, then the write is ignored.
> > OK, I did some archaeology (it has been a while since when I wrote that code.)
> > 
> > Long story short:
> > When simulating in-order processors, you should never end up in a situation where the number of cycles left is UNKNOWN_CYCLES. Your code was not wrong. Adding a `continue` is also OK (but not necessarily better). Ideally, you could just `assert` that the number of cycles is different than UNKNOWN_CYCLES. Up to you.
> > 
> > Long story:
> > Each register write is associated with a `CyclesLeft` quantity. The number of cycles left is unknown until the instruction is issued. Only at that point, we know exactly how many cycles are left before writes are committed to the register file.
> > 
> > On instruction issue, dependent instructions get notified, so that the CyclesLeft quantities associated with their register reads are also updated.
> > 
> > This is important when simulating an out-of-order backend, because the scheduler may contain a chain of dependent instructions, and not all the instructions of that chain are in a "ready" state.
> > 
> > The relevant code is in Instruction.cpp (see for example method `WriteState::onInstructionIssued`).
> > 
> > Back to our case:
> > 
> > For in-order processors, instructions are always issued in-order, and instructions are not buffered internally by a scheduler. So the "dispatch" and "issue" are basically the same event.
> > 
> > In the presence of data dependencies, instructions are artificially delayed until all inputs become available (i.e. inputs have a known number of cycles left). Only at that point, instructions are issued to the underlying execution units.
> > 
> > By construction, you cannot end up in a situation where the "current instruction" depends on an older instruction which hasn't started execution yet.
> > 
> > To put it in another way: By construction, it is always guaranteed that "older instruction" have already started execution. Therefore, the CyclesLeft value from each write is always known.
> Thanks for the confirmation. This matches my analysis as well.
> `Instruction::execute` always updates CyclesLeft, so we never have
> UNKNOWN_CYCLES here. Let's keep the handling in case this changes at some point.
> 
Sounds good.

Thanks for working on this feature!

-Andrea


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

https://reviews.llvm.org/D94928