[PATCH] D63885: [LOOPINFO] Introduce the loop guard API.

[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:737
+  /// dominates the loop header, and the other successor postdominates the loop
+  /// header, i.e. the branch instruction \p BI is a loop guard.
+  bool isLoopGuardBranch(const BranchInst &BI, const DominatorTree &DT,
----------------
Whitney wrote:
> reames wrote:
> > Whitney wrote:
> > > reames wrote:
> > > > Whitney wrote:
> > > > > reames wrote:
> > > > > > Your definition would seem to disallow:
> > > > > > if (C) return;
> > > > > > for (...) {}
> > > > > > 
> > > > > > That is C wouldn't be considered a guard condition.  Intentional?  
> > > > > I think this is fine, in most cases, LLVM tries to common the return blocks, so `if (C)` will jump to the common exit block, and this algorithm will work. Although we can intensionally write LLVMIR with multiple return blocks, I don't think it will be the common case. 
> > > > Put two calls on the return paths to different targets and we don't merge them.
> > > Do you mind giving me an example? I tried the code below, and that doesn't work. 
> > > ```
> > > void bar1();
> > > void bar2();
> > > int foo(bool cond, int *A) {
> > >   if (cond) return 1;
> > >   for (long i = 0; i < 100; ++i) {
> > >     A[i] = 0;
> > >   }
> > >   bar1();
> > >   bar2();
> > >   return 0;
> > > }
> > > ```
> > void bar1();
> > void bar2();
> > int foo(bool cond, int *A) {
> >   if (cond) { bar1(); return 1; }
> >   for (long i = 0; i < 100; ++i) {
> >     A[i] = 0;
> >   }
> >   bar2();
> >   return 0;
> > }
> With the code above, I ran it through clang (noopt), and got:
> ```
>   br i1 %tobool, label %if.then, label %if.end
> 
> if.then:                                          ; preds = %entry
>   call void @_Z4bar1v()
>   store i32 1, i32* %retval, align 4
>   br label %return
> 
> if.end:                                           ; preds = %entry
>   store i64 0, i64* %i, align 8
>   br label %for.cond
> 
> for.cond:                                         ; preds = %for.inc, %if.end
>   %1 = load i64, i64* %i, align 8
>   %cmp = icmp slt i64 %1, 100
>   br i1 %cmp, label %for.body, label %for.end
> 
> for.body:                                         ; preds = %for.cond
>   %2 = load i32*, i32** %A.addr, align 8
>   %3 = load i64, i64* %i, align 8
>   %arrayidx = getelementptr inbounds i32, i32* %2, i64 %3
>   store i32 0, i32* %arrayidx, align 4
>   br label %for.inc
> 
> for.inc:                                          ; preds = %for.body
>   %4 = load i64, i64* %i, align 8
>   %inc = add nsw i64 %4, 1
>   store i64 %inc, i64* %i, align 8
>   br label %for.cond
> 
> for.end:                                          ; preds = %for.cond
>   call void @_Z4bar2v()
>   store i32 0, i32* %retval, align 4
>   br label %return
> 
> return:                                           ; preds = %for.end, %if.then
>   %5 = load i32, i32* %retval, align 4
>   ret i32 %5
> ```
> 
> Looks like the two returns are common into one return block.
> Put two calls on the return paths to different targets and we don't merge them.
@reames Do you have another example which I should pay attention on?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:415
+
+      if (!isGuaranteedToTransferExecutionToSuccessor(Succ))
+        return true;
----------------
Meinersbur wrote:
> Whitney wrote:
> > Meinersbur wrote:
> > > If you want to disallow throwing instructions, how did  you conclude that `isGuaranteedToTransferExecutionToSuccessor` is what you want and not `->mayThrow`?
> > > 
> > > There might be dynamically endless loops as well that isGuaranteedToTransferExecutionToSuccessor does not check (that is, only if the endless loop was inside a call).
> > I chose `isGuaranteedToTransferExecutionToSuccessor` instead of `->mayThrow` is because it consider more cases, for example it checks if memory operation trap. `isGuaranteedToTransferExecutionToSuccessor` is a superset of `->mayThrow`. 
> > 
> > What is dynamically endless loops? 
> > What is dynamically endless loops?
> ```
> header:
>   %cmp = icmp ule i32 0, %a ; anything that always returns true but LLVM cannot figure out
>   br i1 %cmp, label %header, label %exit
> 
> exit:
> ```
I would like to make sure I understand the concern correctly. Is the concern that the guard guards an dynamically endless loops before the loop of interested?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885