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

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
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 {
----------------
andreadb wrote:
> 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?
> 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?

Interesting...I haven't done such measurement and I didn't know vtable dispatching in this case would cause noticeable regression. I'll look into it.

> 
> 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.
> 

Right, if vtable is the bottleneck here, another similar solution can be:
```
template <class SubClass> SourceMgr {
  bool hasNext() const {
    return static_cast<const SubClass *>(this)->hasNext();
  }
};

class MySourceMgr : public SourceMgr<MySourceMgr> {
  bool hasNext() const { /*not a virtual function*/ }
};
```

Then consumers of SourceMgr need to accept something with a template. Which is not a big problem if we only use it in few places (i.e. less fiddling with template arguments) so I'm with the idea you put below to limit as many SourceMgr usages as possible, for instance in CustomBehavior. That said, for EntryStage and InOrderIssueStage, we need to move lots of code from .cpp file to the header file since we're taking template version of SourceMgr, I'm wondering what's your thoughts about this?

> 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.

Sounds reasonable to me.

> 
> 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;
+
----------------
andreadb wrote:
> Is there a reason why you want to add this method?
Originally we were using begin/end methods here, but with the current scheme (i.e. SourceMgr as an abstract class) it's easier to have such `getInstructions` rather than worry about types for the iterator (returning from begin/end) -- It's not impossible to use begin/end, it's just easier to returning an ArrayRef. Also, it works just fine with current usages. Do you think it's better to use begin/end instead?


================
Comment at: llvm/include/llvm/MCA/Stages/Stage.h:21
 #include <set>
+#include <system_error>
 
----------------
andreadb wrote:
> Is there a reason why you use <system_error> for your new Error type?
> 
I didn't know there is `llvm::inconvertibleErrorCode()` when I first wrote this code. I'll change to use `inconvertibleErrorCode()` instead.


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

https://reviews.llvm.org/D127083