[PATCH] D48162: [GSoC] Schedule tree performance.

[lorenzo chelini via Phabricator via llvm-commits]

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


================
Comment at: include/polly/ScheduleOptimizer.h:45
+
+struct Skeleton {
+  std::vector<Reference> References;
----------------
bollu wrote:
> bollu wrote:
> > bollu wrote:
> > > I am not sure I like the use of `struct` here. Usually, it is use to signal a POD (poor old data type) with no complex constructor. In this case, since it owns an `std::vector` (or may own a `SmallVector`), I would prefer it being tagged `class`.
> > > 
> > > You could still make everything `public` if that's what you wanted by making it a `struct`.
> > Also, why `Skeleton`? Is this some name in the paper?
> > 
> > Please add a line of description above why this struct is called `Skeleton` and what it tracks.
> Similarly, can one have a `Skeleton` without a `name`? It may make sense to add a constructor and a method to `add` and `get` references, but not _remove_ them (IIRC, there is no removal of references, correct?)
> 
> In general, having access control would be nice!
I added a bit of description for the Skeleton class. Each Skeleton contains the access pattern for a given Stmt.


================
Comment at: include/polly/ScheduleOptimizer.h:45
+
+struct Skeleton {
+  std::vector<Reference> References;
----------------
chelini wrote:
> bollu wrote:
> > bollu wrote:
> > > bollu wrote:
> > > > I am not sure I like the use of `struct` here. Usually, it is use to signal a POD (poor old data type) with no complex constructor. In this case, since it owns an `std::vector` (or may own a `SmallVector`), I would prefer it being tagged `class`.
> > > > 
> > > > You could still make everything `public` if that's what you wanted by making it a `struct`.
> > > Also, why `Skeleton`? Is this some name in the paper?
> > > 
> > > Please add a line of description above why this struct is called `Skeleton` and what it tracks.
> > Similarly, can one have a `Skeleton` without a `name`? It may make sense to add a constructor and a method to `add` and `get` references, but not _remove_ them (IIRC, there is no removal of references, correct?)
> > 
> > In general, having access control would be nice!
> I added a bit of description for the Skeleton class. Each Skeleton contains the access pattern for a given Stmt.
Thanks for the comment. I am planning in the future to add more flexibility in the Skeleton class. For now I would like to keep it like this.


================
Comment at: include/polly/ScheduleOptimizer.h:79
 /// optimizer.
 struct OptimizerAdditionalInfoTy {
   const llvm::TargetTransformInfo *TTI;
----------------
bollu wrote:
> Is there some better name possible for what this "additional" info is? Something like, `OptimizerClassifierInfo`? (That is also not a great name, but I'd like to have something a little more descriptive)
The structure was already there. I just used the struct to pass my Skeleton vector to the optimizer. Indeed I think the name is a bit too generic. I will think about possible alternatives.


https://reviews.llvm.org/D48162