Add option -unroll-runtime-other-exit-predictable to assume the non latch exit block to be predictable.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 507 | IIUC this is basically trying to avoid the check for the "deoptimize" call on line 514, right? In that case can we just add this check to that line and update the comments on line 512 with something like: ". When UnrollRuntimeAtMostTwoExits is specified we assume the other exit branch is predictable even if it has no deoptimize call"? | |
| llvm/test/Transforms/LoopUnroll/runtime-loop-at-most-two-exits.ll | ||
| 99 | how come it is still unrolled? | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 507 | I would have expected that when OtherExits is empty, we are not in a multi-exit-loop scenario so the result of this function won't matter...but I guess it's better to be cautious and explicitly handle that case. | |
Comment Actions
What is the motivation for the switch? Why wouldn't we want to unroll loops with 3 or more exits provided we can and the heuristic sais its profitable? Why is the cutoff at 3? Could we also pass the cutoff-point as an option?
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 499–502 | 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; | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 57 | 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. | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 57 | Yes. I suppose a value of say 10 would be big enough for most cases....the bigger the number of exiting blocks, the less likely it will be profitable to unroll. | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 57 | ok, I can put a review for that after this patch get landed. | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 490–492 | 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? | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 57 |
...just to clarify this, as Michael pointed out my original intention was to have 0 mean no cutoff (ie any number of exit/exiting blocks are allowed)....but the same thing can effectively be achieved with a large value specified for the option. | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 490–492 | Current behaviour: If we use this code: if (UnrollRuntimeCutoffPoint == 0 ||
(ExitingBlocks.size() <= UnrollRuntimeCutoffPoint &&
OtherExits.size() < UnrollRuntimeCutoffPoint))
return true;-unroll-runtime-cutoff-point==0 => return true 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. | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 490–492 | 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? | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 499–501 | This seems to make sense, but isn't this already covered by !L->getExitingBlock() || OtherExits.size() in the function that calls canProfitablyUnrollMultiExitLoop? (I think we could keep it here, just to make it more explicit) | |
| 508–509 | Looks sensible to me. @bmahjour ? | |
| llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp | ||
|---|---|---|
| 508–509 | LGTM too. | |
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.