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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D127083#3561836 <https://reviews.llvm.org/D127083#3561836>, @myhsu wrote:

>> Did you notice a perf regression after this change on normal llvm-mca runs with several iterations?
>
> Following up on the potential performance regression caused by abstracting away SourceMgr, here is my experiment:
> <snip>
>
> If we look into the number of instructions, a more stable metric, there is an increase on that, but it only accounts about 0.4%.

That's great thanks.

If that's the case, then I am less worried about using a base class with just pure virtual methods.



================
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;
+
----------------
myhsu wrote:
> 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?
In principle, I prefer using ArrayRef instead using begin/end iterators.

The only problem with ArrayRef (at least in this context) is that it assumes that the underlying data structure in SourceMgr can be seen as an array-like struct.

In your patch, the std::deque breaks that assumption. As the docs say, std::deque doesn't guarantee that elements are stored contiguously.

For that reason, having that method returning an ArrayRef is a bit limiting.
Ideally, what you really want is the ability to return an "iterator range".

I noticed that llvm provides a class iterator_range. Maybe using an llvm::iterator_range could be a compromise?


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

https://reviews.llvm.org/D127083