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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

Hi Min,



================
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:
> > 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.
Right. I see the problem.

If you can't find anything better, then your current approach (i.e. marking getInstructions as unimplemented) looks good to me.

I don't have any other requests.

Thanks!


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

https://reviews.llvm.org/D127083