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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/polly/ZoneAlgo.h:326
+  /// Is @p MA a PHI READ access that can be normalized?
+  bool isNormalizable(MemoryAccess *MA);
+
----------------
bollu wrote:
> 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)
@see #ZoneAlgorithm::NormalizeMap which explains what is going on.

I agree that there might be a better name than "normalize", however I am 80% satisfied with the name. Due to the existence of PHIs, there are multiple ValInsts representing the same value. "Normalization" is the process of keeping (ideally) one of these. 

"Computable" as used in `ComputedPHIs` might be an alternative. However, `PHINode`s are also not the only source of normalization. For instance, a LoadInst might always read a previously stored ValInst. We could normalize the ValInst for the loaded value by the ValInst that was stored before.

`isUnderstandablePHIRead` kind of implies to me that other PHIs are not understandable, what would that mean?


================
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;
----------------
bollu wrote:
> 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.
I did the change, but note that IMHO the `const` here is useless. The `llvm::PHINode` structure has not been designed around constness. E.g.
```
cast<PHINode>(Cur->op_begin()->getUser())
```
gets you a const-free object without any const-cast, the `const` gives a false sense that the object cannot be modified and adds clutter to declarations.

Generally, `llvm::Value` objects are identified by their address. That is, even after modifying it, we still means the same SSA value, so `const` does not accomplish anything,. This is different for e.g. `SCEV`, `DenseSet`.

In the past (eg. rL292480) we already had to remove `const` keywords that were added just because it could be added; it forced us to insert `const_cast`s if we wanted to change code.


================
Comment at: lib/Transform/ZoneAlgo.cpp:989
+          PerPHI.apply_domain(PHIValInst).apply_range(IncomingValInsts);
+      assert(!PHIMap.is_single_valued().is_false());
+
----------------
bollu wrote:
> Double negative: consider `assert(PHIMap.is_single_valued().is_true())`
`is_single_valued()` returns a tri-valued `isl::boolean`. The intended meaning is
```
assert(PHIMap.is_single_valued().is_true_or_error());
```
Unfortunately `is_true_or_error()` did not make it into the isl API. Another alternative could be
```
assert(PHIMap.is_single_valued().is_error() || PHIMap.is_single_valued().is_true());
```
Unfortunately the quota limit could run out in the second call of `is_single_valued()`, such that the calls return different values.


https://reviews.llvm.org/D39333