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

[Whitney via Phabricator via llvm-commits]

Whitney marked 3 inline comments as done.
Whitney added inline comments.


================
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
----------------
jdoerfert wrote:
> 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? 
I modified getDirection() which get the direction using SCEVAddRecExpr, getCanonicalPredicate() will return BAD_ICMP_PREDICATE if enable to confirm the direction when the predicate is NE or EQ.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565