[PATCH] D48162: [GSoC] Schedule tree performance.
Siddharth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 16 12:15:02 PDT 2018
bollu added a subscriber: philip.pfaffe.
bollu added a comment.
Thanks for the patch! I've added some comments about general LLVM style, and some personal choices. Feel free to ignore the personal choices :)
- I value `const` highly, but this is not true across LLVM and Polly.
- Also, please run the file through `ninja polly-update-format`, It'll fix the formatting according to the formatting guidelines that are enforced using `clang-format`. I think in some places, the formatting is off?
================
Comment at: include/polly/ScheduleOptimizer.h:28
+ std::string Name;
+ std::vector<isl::val> ElementAccessed;
+ std::vector<bool> HasStride;
----------------
Consider using `llvm::SmallVector`, which is usually more performant.
================
Comment at: include/polly/ScheduleOptimizer.h:29
+ std::vector<isl::val> ElementAccessed;
+ std::vector<bool> HasStride;
+ isl::set Domain;
----------------
consider `SmallVector`?
================
Comment at: include/polly/ScheduleOptimizer.h:31
+ isl::set Domain;
+ std::vector<unsigned> LoopOrder;
+
----------------
Ditto. `SmallVector`
================
Comment at: include/polly/ScheduleOptimizer.h:33
+
+ enum ReferenceType {
+ SINGLE_ELEMENT,
----------------
Consider `enum class` over `enum`, it is slightly stronger with respect to typecasting, and is better namespaced.
================
Comment at: include/polly/ScheduleOptimizer.h:45
+
+struct Skeleton {
+ std::vector<Reference> References;
----------------
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`.
================
Comment at: include/polly/ScheduleOptimizer.h:45
+
+struct Skeleton {
+ std::vector<Reference> References;
----------------
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.
================
Comment at: include/polly/ScheduleOptimizer.h:46
+struct Skeleton {
+ std::vector<Reference> References;
+ const char *StmtBaseName;
----------------
consider `SmallVector`?
================
Comment at: include/polly/ScheduleOptimizer.h:47
+ std::vector<Reference> References;
+ const char *StmtBaseName;
+};
----------------
I am a little hesitant about `char *`, as mentioned below. consider `std::string`?
================
Comment at: include/polly/ScheduleOptimizer.h:79
/// optimizer.
struct OptimizerAdditionalInfoTy {
const llvm::TargetTransformInfo *TTI;
----------------
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)
================
Comment at: include/polly/ScheduleOptimizer.h:82
const Dependences *D;
+ const std::vector<Skeleton> &Skeletons;
};
----------------
`SmallVector` :)
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1328
+static bool checkForStreamingAccesses(const std::vector<Reference> &References) {
+
----------------
Consider something like `containsStreamingAccess`? I feel this reads better, when used with `if`:
`if(checkForStreamingAccess(...))`
versus
`if(containsStreamingAccess(...))`
`check` sounds more like a "command" / "imperative", while this function is more of a "predicate".
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1330
+
+ for(auto it = References.begin(); it != References.end(); it++) {
+ if(it->Type != Reference::CHUNK && it->Type != Reference::SINGLE_ELEMENT)
----------------
consider using range-based for?
```lang=cpp
for (auto it : References) { ... }
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1335
+
+ assert(References.size() != 1);
+
----------------
consider adding desrciption for why this assert must hold:
```lang=cpp
assert (References.size() != 1 && "reason");
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1359
+ return false;
+ isl::map NewPartialSchedule = isl::map::from_union_map(PartialSchedule);
+ isl::id InputDimsId = NewPartialSchedule.get_tuple_id(isl::dim::in);
----------------
I would prefer a more descriptive name, something like `SinglePartialSchedule` to emphasize that this code is pulling out a single `isl::map` from an `isl::union_map`
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1364
+ std::vector<Skeleton>::const_iterator it = SK.begin();
+ for(; it != SK.end(); it++) {
+ if(strcmp(it->StmtBaseName, Stmt->getBaseName()) != 0)
----------------
consider range-based for?
```lang=cpp
for(auto it : SK) { ...}
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1527
+
+static void getDomain(MemoryAccess *MemAcc, isl::set &Dom) {
+
----------------
1. Consider returning the `Dom`?
2. Consider `const MemoryAccess *MemAcc`, since there seems to be no modification of the `MemAcc`?
````lang=cpp
static isl::set getDomain(MemoryAccess *MemAcc)
````
and assign `Dom` at the call site?
```lang=cpp
Dom = getDomain(...)
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1577
+
+static void getStep(MemoryAccess *MemAcc, isl::map &Sched,
+ /*unsigned LoopLevel,*/ std::vector<bool> &HasStride) {
----------------
Consider returning an `SmallVector<bool>` rather than passing a parameter to be initialized? That is, I would prefer the type signature being
```lang=cpp
static SmallVector<bool> getStep(MemoryAccess *MemAcc, isl::map &Sched)
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1698
+
+static bool accessSingleElement(std::vector<isl::val> &E) {
+ bool isElement = true;
----------------
consider a more "predicate" sounding name, like `doesAccessSingleElement`.
Also, `SmallVector`, and maybe `const isl::val`?
```lang=cpp
static bool doesAccessSingleElement(std::vector<const isl::val> &E)
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1715
+ std::vector<Reference>::iterator it = R.begin();
+ while (it != R.end()) {
+ if (!it->HasStride[0] && it->AccessType == 1) {
----------------
Consider range-based-for loop
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1759
+
+static void printStructure(std::vector<Reference> &R, const char *StmtBaseName) {
+ assert(!R.empty() && "Structure is empty");
----------------
This actually seems useful to have :) I am not sure what our policy is on keeping around debug printing functions, so ping @grosser @Meinersbur @philip.pfaffe and @pollydev
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1820
+
+static void classifyReference(ScopStmt &Stmt,
+ std::vector<Reference> &References) {
----------------
Consider returning `SmallVector<Reference>`? It makes a function "pure" in my eyes, rather than having to mutate some external data.
Also, is it possible for the reference to `ScopStmt` to be `const`? From the name ("classify"), it should not need to modify anything in `ScopStmt`, correct?
So, consider:
```lang=cpp
static SmallVector<Reference, 4> classifyReference(const ScopStmt &Stmt)
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1884
+ /*LoopLevel,*/ References[Index].HasStride);
+ Index++;
+ }
----------------
Consider constructing the `Reference` object first and finally calling `push_back`? This way, there is no need to track index.
That is,
```lang=cpp
for (auto *MemA = Accesses.begin(); MemA != Accesses.end(); MemA++) {
...
Reference R;
R.AccessType = MemAccessPtr->getType();
R.Name = MemAccessPtr->getLatestScopArrayInfo()->getName();
R.Domain = getDomain(MemAccessPtr);
...
References.push_back(R);
}
```
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1893
+
+static bool isProfitableStmt(ScopStmt &Stmt) {
+
----------------
consider `const ScopStmt`?
================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1934
+ SK.push_back(Skeleton());
+ SK[Index].StmtBaseName = Stmt.getBaseName();
+ classifyReference(Stmt, SK[Index].References);
----------------
Correct me if I am wrong, but the string obtained from `Stmt.getBaseName()` is owned by the `ScopStmt` correct? [Here is the implementation](https://github.com/llvm-mirror/polly/blob/master/lib/Analysis/ScopInfo.cpp#L1722) which returns the `char*` owned by `std::string ScopStmt::BaseName`
So, I would prefer to make a copy into `Skeleton`, rather than hold a `const char *`.
https://reviews.llvm.org/D48162
More information about the llvm-commits
mailing list