[PATCH] D39333: [Polly][ZoneAlgo/ForwardOpTree] Normalize PHIs to their known incoming values.

[Siddharth via Phabricator via llvm-commits]

bollu added inline comments.


================
Comment at: include/polly/ZoneAlgo.h:118
+  /// Computing them would require a polyhedral transitive closure operation,
+  /// for which isl may only return an approximation. We correctness, we always
+  /// require an exact result. Hence, we exclude such PHIs.
----------------
"We correctness" -> "We want correctness", perhaps?


================
Comment at: include/polly/ZoneAlgo.h:326
+  /// Is @p MA a PHI READ access that can be normalized?
+  bool isNormalizable(MemoryAccess *MA);
+
----------------
What does `Normalized` exactly refer to, here? I assume it refers to the PHI node normalization, but can we look for a better name? `Normal` is one of the most overloaded words there is `:)`. Something like, `isUnderstandablePHIRead`? (I don't like that name either)


================
Comment at: lib/Transform/ZoneAlgo.cpp:487
+/// In this example, %phi1 is recursive, but %phi2 is not.
+static bool isRecursivePHI(PHINode *PHI) {
+  SmallVector<PHINode *, 8> Worklist;
----------------
consider `const PHINode *PHI`? From what I can see, no mutation is required of the `PHI`, and all methods used from the Phi node have `const` versions.


================
Comment at: lib/Transform/ZoneAlgo.cpp:771
+/// value.
+static isl::union_map normalizeValInst(isl::union_map Input,
+                                       isl::union_map NormalizedPHIs,
----------------
Could you please document what `Input` and `NormalizedPHIs` are expected to look like?


================
Comment at: lib/Transform/ZoneAlgo.cpp:773
+                                       isl::union_map NormalizedPHIs,
+                                       DenseSet<PHINode *> &TranslatedPHIs) {
+  isl::union_map Result = isl::union_map::empty(Input.get_space());
----------------
consider `const DenseSet<PHINode *> &TranslatedPHIs`? It's only used to `count`.


================
Comment at: lib/Transform/ZoneAlgo.cpp:989
+          PerPHI.apply_domain(PHIValInst).apply_range(IncomingValInsts);
+      assert(!PHIMap.is_single_valued().is_false());
+
----------------
Double negative: consider `assert(PHIMap.is_single_valued().is_true())`


https://reviews.llvm.org/D39333