[PATCH] D97747: [LoopUnrollRuntime] Add option to assume the non latch exit block to be predictable.

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:491-493
+  if (ExitingBlocks.size() <= UnrollRuntimeCutoffPoint &&
+      OtherExits.size() < UnrollRuntimeCutoffPoint)
+    return true;
----------------
Whitney wrote:
> Meinersbur wrote:
> > I think @bmahjour's suggestion (and mine) was to completely disable UnrollRuntimeCutoffPoint if it is `0`
> > ```
> > if (UnrollRuntimeCutoffPoint == 0 ||
> >       (ExitingBlocks.size() <= UnrollRuntimeCutoffPoint &&
> >       OtherExits.size() < UnrollRuntimeCutoffPoint))
> >     return true;
> > ```
> > 
> > I don't really understand why it bounds both, ExitingBlock and OtherExists (instead of only one of them or the sum of such blocks). Could you explain?
> > 
> > @bmahjour's other point is that it makes `UnrollRuntimeMultiExit` redundant: `-unroll-runtime-multi-exit=true` is equivalent to `-unroll-runtime-cutoff-point=0` and  -unroll-runtime-multi-exit=false` is equivalent to `-unroll-runtime-cutoff-point=1` (I think in this case I think `canProfitablyUnrollMultiExitLoop` would not be called anyway). Do I understand correctly that you would remove the `-unroll-runtime-multi-exit` in a followup-patch?
> Current behaviour:
> -unroll-runtime-multi-exit==true => return true
> -unroll-runtime-multi-exit==false => return false
> -unroll-runtime-multi-exit not specified => return (ExitingBlocks.size() <= 2 && OtherExits.size() == 1 && OtherExits[0]->getTerminatingDeoptimizeCall())
> 
> If we use this code:
> ```
> if (UnrollRuntimeCutoffPoint == 0 ||
>       (ExitingBlocks.size() <= UnrollRuntimeCutoffPoint &&
>       OtherExits.size() < UnrollRuntimeCutoffPoint))
>     return true;
> ```
> -unroll-runtime-cutoff-point==0 => return true
> -unroll-runtime-cutoff-point==1 => return (ExitingBlocks.size() <= 1 OtherExits.size() < 1) || (ExitingBlocks.size() <= 2 && OtherExits.size() == 1 && OtherExits[0]->getTerminatingDeoptimizeCall())
> There is no initial value that can keep the current behaviour of (ExitingBlocks.size() <= 2 && OtherExits.size() == 1 && OtherExits[0]->getTerminatingDeoptimizeCall()).
> 
> You are right that there is no direct relation between number of exiting blocks and other exits, so we would have to use two cutoff points.
> 
> After rethinking about it more, to achieve my original need, which is to allow unrolling a loop with at most 2 exiting blocks and at most 1 other exit block, I added an option to assume the other exit is predictable.
ok, this diff is now more like what I had in mind in the beginning (see https://reviews.llvm.org/D97747#inline-916962). Could you please also update the comments on line 506?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97747