[PATCH] D97747: [LoopUnrollRuntime] Add option to unroll loops with at most two exit/exiting blocks.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:57
+             "when epilog is generated"));
+static cl::opt<bool> UnrollRuntimeEnableCutoffPoint(
+    "unroll-runtime-enable-cutoff-point", cl::init(false), cl::Hidden,
----------------
bmahjour wrote:
> I think we can avoid this extra option by initializing the Cutoff to 0. Any number larger than 0 would mean a valid cutoff. (ie 0 means no cutoff enabled, 1 means no multi-exit, 2 means at most 2 exits, etc). It would also render `UnrollRuntimeMultiExit` obsolete, which can be replaced in this patch or a subsequent one.
Why `UnrollRuntimeMultiExit` is obsolete? Do you mean the user of `UnrollRuntimeMultiExit` can put a very big number?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:500-503
+  // Only enable unrolling of loops that have at most a single side exit other
+  // than the normal LatchExit.
+  if (OtherExits.size() > 1)
+    return false;
----------------
Meinersbur wrote:
> `OtherExits.size() > 1` will also make the `getTerminatingDeoptimizeCall` heuristic fail, so this is not a semantics change. However, I find the structure confusing, especially if we want to add more conditions that enable the optimization. Did you consider the following structure?
> 
> ```
> if (OtherExits.size() <= 1 && UnrollRuntimeAtMostTwoExits)
>   return true;
> 
> if (OtherExits.size() == 1 && OtherExits[0]->getTerminatingDeoptimizeCall())
>   return true;
> 
> // More conditions can be added here
>   
> return false;
> ```
What do you think of the current structure? I have added a cutoff option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97747