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

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

myhsu marked 2 inline comments as done.
myhsu added inline comments.


================
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:
> 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?
> In your patch, the std::deque breaks that assumption. As the docs say, std::deque doesn't guarantee that elements are stored contiguously.

Correct, though `IncrementalSourceMgr::getInstructions` will eventually be marked as unimplemented / not applicable. Because when the users decide to recycle instructions, it's really hard to return a meaningful sequence of instructions. That said, we can still return the sequence of instructions stored in `InstStorage` anyway as a best-effort result.

> Ideally, what you really want is the ability to return an "iterator range".

Agree, though this goes back to a previous concern where we need to abstract away the type of iterator (as iterator_range still takes the type of iterator as its template argument). I will do some explorations on this topic.


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

https://reviews.llvm.org/D127083