[PATCH] D60565: [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:609
+  /// else return nullptr.
+  std::unique_ptr<LoopBounds> getBounds(ScalarEvolution *SE = nullptr) const;
+
----------------
Whitney wrote:
> fhahn wrote:
> > Do we need dynamic allocations here? The LoopBounds struct is small and trivially movable/copyable, so just returning it as a value would be simpler and probably faster.
> The reason I wanted to return a unique_ptr is getBounds() may return nullptr if it doesn't find the induction variable, or unable to create the bound. And with unique_ptr, it doesn't need to worry about cleaning the underlining object of a pointer. What do you think?
I usually do something like:
 Pass an uninitialized object as a parameter and the boolean return value will tell if it was initialized and is now valid.
However, there are various pros and cons for most solutions. I personally don't have strong feelings about how (especially not wrt. performance), the only thing I want is simple code that is easy to reason about. That said, maybe an `Optional<LoopBounds>` is what you are really after?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:584
+    /// - the LHS of the latch comparison is StepInst
+    ICmpInst::Predicate getCanonicalPredicate() const;
+    /// An enum for the direction of the loop
----------------
Whitney wrote:
> jdoerfert wrote:
> > This definition seems to lose given that there are various corner cases, e.g., wrapping induction variables.
> Are you talking about the getCanonicalPredicate()? I am trying to understand the problem. Can you please give me an example which doesn't work?
> This function is called when there exists an induction variable, a corresponding compare instruction, and a corresponding step instruction. And it is returning the predicate of the compare instruction when the first successor of the latch branch is the loop header. 
Let me try to come up with one that showcases my concerns but is probably not the only problematic case.

You probably have in mind something like:

```
// Canonical pred: '<'.
// Direction of u: 'increasing'
for (int8_t u = 0; u < 127; u++)
  body(u);
```

But what happens if you have:

```
// I don't even know what "the canonical pred" is.
// 'Direction' of u increases, then decreases (wrap around), then increases.
for (uint8_t u = 128; u != 127; u++)
  body(u);

```

I think there are two solutions to these kinds of problems:
  1) Think through the overflow cases and return "the right thing". Though, some of these queries do not have a "canonical" answer if overflows are possible.
  2) Exclude all, or some, overflow cases by preventing us to build a `LoopBounds` object. This is probably the short term solution we should first commit.

Disclaimer:
Maybe you somewhere handle overflow problems and I just missed it. Especially the new SE based reasoning is probably less prone to these problems. In the above example is no problem,
can you add and(/or point me at) tests to verify we "do the right thing" in overflow cases? 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60565/new/

https://reviews.llvm.org/D60565