[PATCH] D31247: [Polly][DeLICM] Known knowledge.

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

thank you for pushing this forward. This is again a pretty large patch, even though the kind of different changes are now a lot smaller. Still, for proper and timely review, it would really help to get these patches smaller and upstream as much as possible independently. I did a first round over this patch, and trying to understand which parts are needed to get the tests working that you contributed. It seems indeed that e.g. VirtualUse is not really needed and to my surprise also the computeKnownFromWrite / computeKnownFromRead functions do not seem to be needed. At least all test cases pass even when I just return an empty known set. Did I make some wrong assumptions or is this functionality not tested and can be added later together with the corresponding test cases. (I stop now my review, to get first some feedback from you).

Best,
Tobias



================
Comment at: include/polly/ScopInfo.h:1334
+    return getRegion()->contains(BB);
+  }
+
----------------
This can probably be committed separately and directly be used and tested in RegionGenerator::addOperandToPHI. No further review needed for this.


================
Comment at: include/polly/Support/VirtualInstruction.h:37
+  enum UseType {
+    // An llvm::Constant.
+    Constant,
----------------
An -> A


================
Comment at: lib/Transform/DeLICM.cpp:391
-  }
-  return nullptr;
 }
----------------
This move also not seem to be needed without VirtualUse.


================
Comment at: lib/Transform/DeLICM.cpp:864
+  /// LoopInfo analysis used to determine whether values are synthesizable.
+  LoopInfo *LI;
+
----------------
Not needed without VirtualUse?


================
Comment at: lib/Transform/DeLICM.cpp:901
+      : IslCtx(S->getSharedIslCtx()), S(S), LI(LI),
+        Schedule(give(S->getSchedule())) {
 
----------------
Not needed without virtual use?


================
Comment at: lib/Transform/DeLICM.cpp:1209
+  ///
+  /// @param Val            The value to look get the instance of.
+  /// @param UserStmt       The statement that uses @p Val. Can be nullptr.
----------------
Grammar: "look get"


================
Comment at: lib/Transform/DeLICM.cpp:1212
+  /// @param IsEntryPHIUser Whether the user is a PHINode in the statement's
+  /// entry block.
+  /// @param Scope          Loop the using instruction resides in.
----------------
I prefer comments to be aligned to the start of the first line (otherwise they mix with the parameters).


================
Comment at: lib/Transform/DeLICM.cpp:1236
+    switch (VUse.getType()) {
+    case VirtualUse::Constant:
+    case VirtualUse::ReadOnly: {
----------------
The only case currently tested seems to be VirtualUse::Constant. If I add a "break" at the beginning of each other case, all tests still pass as expected. Maybe we can first introduce the constant case, without virtual use and then look again at the virtual use?


================
Comment at: lib/Transform/DeLICM.cpp:1238
+    case VirtualUse::ReadOnly: {
+      // The definition does not depend the statement which uses it.
+      auto ValSet = makeValueSet(Val);
----------------
on the


================
Comment at: lib/Transform/DeLICM.cpp:1941
+    if (WrittenValUse.isInter())
+      Worklist.push_back(WrittenValUse.getMemAccess());
     else
----------------
If I revert this change and use the original getInputAccessOf function, all tests still pass. This and the use where we check for Val to be a constant are the only uses of VirtualUse. If we drop them, this patch would become more targeted.


================
Comment at: lib/Transform/DeLICM.cpp:2017
 
-  /// Determine when an array element is written to.
+  isl::union_map computeKnownFromWrite() const {
+
----------------
If I add  return makeEmptyUnionMap(); all tests still pass.


================
Comment at: lib/Transform/DeLICM.cpp:2030
+
+  isl::union_map computeKnownFromRead() const {
+    // { [Element[] -> Scatter[]] -> DomainWrite[] }
----------------
If I add  return makeEmptyUnionMap(); all tests still pass.


================
Comment at: lib/Transform/DeLICM.cpp:2117
 public:
-  DeLICMImpl(Scop *S) : ZoneAlgorithm(S) {}
+  DeLICMImpl(Scop *S, LoopInfo *LI) : ZoneAlgorithm(S, LI) {}
 
----------------
Not needed without virtual Statement.


================
Comment at: lib/Transform/DeLICM.cpp:2250
+    Impl = make_unique<DeLICMImpl>(
+        &S, &getAnalysis<LoopInfoWrapperPass>().getLoopInfo());
 
----------------
Not needed without VirtualStmt.


================
Comment at: lib/Transform/DeLICM.cpp:2270
     AU.addRequiredTransitive<ScopInfoRegionPass>();
+    AU.addRequired<LoopInfoWrapperPass>();
     AU.setPreservesAll();
----------------
Not needed without VirtualStmt.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:127
+  // auto OtherSpace = give( isl_set_universe(  isl_space_set_tuple_id(
+  // isl_space_set_alloc(Ctx.get(), 0, 0 ), isl_dim_set, OtherId.copy())) );
+
----------------
I assume we do not need this commented code to be committed.


https://reviews.llvm.org/D31247