[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)
Tobias Grosser via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 19 04:37:52 PDT 2016
grosser added a comment.
Hi Michael,
I just finished the first pass over everything that is not core DeLICM. I will now look closer into the actual DeLICM implementation.
Best,
Tobias
================
Comment at: include/polly/DeLICM.h:34
@@ +33,3 @@
+///
+/// The reaching definition of an array element at a specific instant is the
+/// statement instance that had written the current element's content. This
----------------
You use 'instance' and 'instant'. I understand what a statement instance or statement instances are, but am not sure what you mean by 'instant'.
================
Comment at: include/polly/DeLICM.h:69
@@ +68,3 @@
+/// @param InclRedef Whether to assume that at the instant where an element
+/// is overwritten, it still contain the old value (any load at
+/// that instant would happen before the overwrite). In this
----------------
containS
================
Comment at: include/polly/DeLICM.h:100
@@ +99,3 @@
+/// @param Schedule { DomainWrite[] -> Scatter[] }
+/// Schedule of (at least) all array writes. Instances no
+/// in @p Writes will be ignored.
----------------
noT
================
Comment at: include/polly/DeLICM.h:124
@@ +123,3 @@
+
+/// Compute the instants where the contents of an array element is not used.
+///
----------------
ARE not used.
================
Comment at: include/polly/DeLICM.h:169
@@ +168,3 @@
+/// @param InclLastRead Whether the instant where an element is last read
+/// counts as unused (the read happens an the beginning
+/// of its instant, and nothing (else) can use it
----------------
AT the beginning
================
Comment at: include/polly/ScopInfo.h:1372
@@ -1366,1 +1371,3 @@
+ void removeSingleMemoryAccess(MemoryAccess *MA);
+
typedef MemoryAccessVec::iterator iterator;
----------------
This seems to be dead code. Can we drop this from the patch?
================
Comment at: include/polly/ScopInfo.h:2455
@@ -2446,1 +2454,3 @@
+ /// When true, also removes statements without side-effects.
+ void simplifySCoP(bool AfterHoisting);
};
----------------
I would suggest to commit this ahead of time, just explaining that we will later use this for DeLICM.
================
Comment at: include/polly/Support/GICHelper.h:273
@@ +272,3 @@
+ ~IslPtr() {
+ if (Obj)
+ Traits::free(Obj);
----------------
Meinersbur wrote:
> This is to allow the compiler to remove the call the `Traits::free` if it knows that `Obj` is always nullptr (eg. because there is an unconditional call to `take()` before). I will commit this separately.
Yes, this makes perfect sense. Please do so for the next revision of the patch.
================
Comment at: include/polly/Support/GICHelper.h:355
@@ -336,2 +354,3 @@
std::string toStr() const { return Traits::to_str(Obj); }
+ void dump() const { llvm::outs() << toStr() << "\n"; }
};
----------------
Meinersbur wrote:
> Will be removed.
This actually seems useful as a debugging tool. I would suggest to commit this separately, though.
================
Comment at: include/polly/Support/GICHelper.h:176
@@ -171,1 +175,3 @@
+ const std::string &Suffix,
+ bool IncludeType = false);
----------------
I cannot find any use of this functionality. Dropping it is a good idea.
================
Comment at: lib/Analysis/ScopInfo.cpp:915
@@ -914,1 +914,3 @@
+llvm::raw_ostream &polly::operator<<(llvm::raw_ostream &OS,
+ const MemoryAccess &MA) {
----------------
Meinersbur wrote:
> The function returns a one-line description of a MemoryAccess. Useful eg. for
> ```
> DEBUG(dbgs() << "Read access: " << *RA);
> DEBUG(dbgs() << "Write access: " << *WA);
> ```
Alright. I would suggest to make the actual formatting/printing of the MemoryAccess class and then only call this functionality from the operator
overloading. Also, if we could find a way to introduce / test this as part of normal Polly, this would be great. Can we e.g. use this to print new access function when importing from jscop? As such, we could introduce this code as separate patch.
================
Comment at: lib/Analysis/ScopInfo.cpp:1119
@@ -1063,3 +1118,3 @@
isl_map_free(NewAccessRelation);
- NewAccessRelation = NewAccess;
+ NewAccessRelation = isl_map_align_params(NewAccess, OriginalDomainSpace);
}
----------------
Meinersbur wrote:
> ISL removes parameters from computed schedules when they are unused. This ensures that `NewAccessRelation` has all parameters available like before. Missing parameters can cause ISL errors further down in the pipeline.
Interesting. I would suggest to commit this separately -- at best with a corresponding jscop test case -- otherwise just clarifying that this will be used and tested later through DeLICM.
================
Comment at: lib/Analysis/ScopInfo.cpp:1783
@@ -1716,1 +1782,3 @@
+}
+
//===----------------------------------------------------------------------===//
----------------
As noted above, this seems to be dead code.
================
Comment at: lib/Analysis/ScopInfo.cpp:3724
@@ -3657,3 +3723,3 @@
return OptimizableStmtsOrLoops > 1;
}
----------------
This seems to be a separate heuristic change. To actually turn on this change, we probably want to do some more evaluation. I would suggest to add a flag that controls this heuristic, which preserves the current behavior, but which can be flipped for DeLICM testing. You can then already commit this change independently.
================
Comment at: lib/CodeGen/BlockGenerators.cpp:473
@@ +472,3 @@
+ isl_set_free(AccDom);
+#endif
+
----------------
This change seems to establish an invariant that already holds today. Please commit this independently.
================
Comment at: lib/CodeGen/BlockGenerators.cpp:481
@@ -460,3 +480,3 @@
"Domination violation");
- BBMap[MA->getBaseAddr()] =
+ BBMap[MA->getAccessValue()] =
Builder.CreateLoad(Address, Address->getName() + ".reload");
----------------
Is this only needed for DeLICM or is this an independent bug fix? In which situations would this change be needed?
================
Comment at: lib/CodeGen/BlockGenerators.cpp:507
@@ -479,1 +506,3 @@
+#endif
+
Value *Val = MA->getAccessValue();
----------------
This seems to be an assertion that should already hold today. Please commit this independently to keep the patch small.
================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:725
@@ -724,1 +724,3 @@
+ assert(!MA->getLatestScopArrayInfo()->getBasePtrOriginSAI() &&
+ "Indirect access codegen not supported");
auto PWAccRel = MA->applyScheduleToAccessRelation(Schedule);
----------------
This assert should already hold today. Please commit it independently.
================
Comment at: lib/Support/GICHelper.cpp:180-182
@@ -179,2 +179,5 @@
replace(str, "=>", "TO");
+ replace(str, "+", ")");
+ replace(str, "%", "");
+ replace(str, "@", "");
}
----------------
Meinersbur wrote:
> Adding the type to ISL names causes `%` and `@` to be in the middle of the string. `+` appears in `llvm::Value`s of floating-point constants.
As types don't seem to be needed, this change can be dropped from this patch, right?
================
Comment at: lib/Support/RegisterPasses.cpp:166
@@ +165,3 @@
+ "polly-enable-delicm",
+ cl::desc("Use scalar-to-array access conversion using non-used zons"),
+ cl::Hidden, cl::init(true), cl::cat(PollyCategory));
----------------
zons -> zones, or alternatively "Eliminate scalar loop carried dependences"
================
Comment at: lib/Transform/DeLICM.cpp:20
@@ +19,3 @@
+// "instant". Instants are lexicographically ordered such that we can defined
+// ranges in the scatter space. We use to flavors of such ranges: Instant sets
+// and zones. An instant set is simply a subset of the scatter space and are
----------------
TWO flavors
================
Comment at: lib/Transform/DeLICM.cpp:21
@@ +20,3 @@
+// ranges in the scatter space. We use to flavors of such ranges: Instant sets
+// and zones. An instant set is simply a subset of the scatter space and are
+// directly stored as isl_sets.
----------------
and IS directly
================
Comment at: lib/Transform/DeLICM.cpp:53
@@ +52,3 @@
+//
+// It sometimes help think about a value of i stored in an isl_set to represent
+// the instant i-0.5 between two integer-valued instants.
----------------
helpS to
================
Comment at: lib/Transform/DeLICM.cpp:58
@@ +57,3 @@
+//
+// The code make use of maps and sets in many different spaces. To not lose
+// track which in which space a set or map is expected to be in, most variables
----------------
makeS use
================
Comment at: lib/Transform/DeLICM.cpp:59
@@ +58,3 @@
+// The code make use of maps and sets in many different spaces. To not lose
+// track which in which space a set or map is expected to be in, most variables
+// holding an ISL reference are annotated in the comments. They roughly follow
----------------
track [WHICH] in which
================
Comment at: lib/Transform/DeLICM.cpp:103
@@ +102,3 @@
+//
+// An annotation { Domain[] -> Scatter[] } therefore means: A map from an
+// statement instance to an instance, aka a schedule. There is only one scatter
----------------
A statement
================
Comment at: lib/Transform/DeLICM.cpp:104
@@ +103,3 @@
+// An annotation { Domain[] -> Scatter[] } therefore means: A map from an
+// statement instance to an instance, aka a schedule. There is only one scatter
+// space, but most of the time multiple statements are processed in one set.
----------------
to an "INSTANCE", in your wording should this not be 'instant'
================
Comment at: lib/Transform/DeLICM.cpp:116
@@ +115,3 @@
+#include "llvm/ADT/Statistic.h"
+#include "llvm/IR/Dominators.h"
+#include "isl/aff.h"
----------------
Is this still needed?
================
Comment at: lib/Transform/DeLICM.cpp:140
@@ +139,3 @@
+ cl::desc("Only collapse to non-scalar array elements"),
+ cl::init(true), cl::cat(PollyCategory), cl::Hidden);
+
----------------
The types and names of these arguments do not match. I would expect a 'MIN' argument to be an integer. Could you check if the naming makes sense?
================
Comment at: test/CMakeLists.txt:163
@@ -162,3 +162,3 @@
FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ
GROUP_EXECUTE WORLD_READ WORLD_EXECUTE)
----------------
Not sure if these changes and the endif change above are really needed or just remainders of committing a slightly different test case. I suggest to either drop them or to commit the necessary changes independently.
================
Comment at: test/CMakeLists.txt:203
@@ +202,2 @@
+add_custom_target(polly-update-tests DEPENDS ${updateable_tests_depends})
+set_target_properties(polly-update-tests PROPERTIES FOLDER "Polly")
----------------
NOTE (to myself): This change is unrelated.
https://reviews.llvm.org/D24716
More information about the llvm-commits
mailing list