[PATCH] D127083: [MCA] Introducing incremental SourceMgr and resumable pipeline

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Min,

Thanks for this patch. I have just started looking at it. I left a few comments inline.

Thanks!



================
Comment at: llvm/include/llvm/MCA/Pipeline.h:54
 
+  enum State {
+    S_Created = 0, // Pipeline was just created. The default state.
----------------
Can you use an `enum class` here?


================
Comment at: llvm/include/llvm/MCA/SourceMgr.h:26-29
 
-class SourceMgr {
+/// Abstracting the input code sequence (a sequence of MCInst) and assigning
+/// unique identifiers to every instruction in the sequence.
+struct SourceMgr {
----------------
This is not ideal, but I can't think of any other obvious way to avoid this refactoring.

Did you notice a perf regression after this change on normal llvm-mca runs with several iterations?

I wonder if you could make SourceMgr a template class, and have it specialised based on whether you want a "circular buffer" behaviour or not.

Ideally, `class SourceMgr` should only be visible to the so-called pipeline entry stage(s). In llvm-mca there are two default entry stages: `EntryStage` (for the out-of-order pipeline) and `InOrderIssueStage` (for the in-order pipeline). Any other module should not use SourceMgr; for those modules, the input assembly sequence can often be easily abstracted using an ArrayRef<MCInst>. This is for example what we do in some Views.

That being said, I am seeing uses of `SourceMgr` in the `CustomBehaviour` API. For example, AMDGPU uses a `CustomBehaviour` to get access to the immutable MCInst sequence and simply iterate over the MCInst. This might have been an oversight/poor design choice from our end, since we don't seem to really need any of the extra SourceMgr functionalities in CustomBehaviour. In fact, passing an ArrayRef<MCInst> would be enough (at least for the AMDGPU).
'
In retrospect, I wonder whether there was a reason for that choice.
@holland11 Would it cause problems if we changed the CustomBehaviour constructor so that it accepts an ArrayRef<MCInst> instead of a SourceMgr? Do you remember why we needed it?


================
Comment at: llvm/include/llvm/MCA/SourceMgr.h:32-34
+  /// Provides a fixed range of \a UniqueInst to iterate.
+  virtual ArrayRef<UniqueInst> getInstructions() const = 0;
+
----------------
Is there a reason why you want to add this method?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127083