[PATCH] D48162: [GSoC] Schedule tree performance.
lorenzo chelini via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 19 08:49:04 PDT 2018
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
More information about the llvm-commits
mailing list