[PATCH] D93764: [LoopUnswitch] Implement first version of partial unswitching.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 313906.
fhahn added a comment.

Updated to use MemorySSA to allow non-clobbering stores in the 'invariant' part. There's a crash while updating MemorySSA, probably because of a new case of loop not accounted in the existing updating. Will fix tomorrow.

In D93764#2472373 <https://reviews.llvm.org/D93764#2472373>, @jonpa wrote:

>> This patch applies the idea from D93734 <https://reviews.llvm.org/D93734> to LoopUnswitch.
>
> One disadvantage here is that LoopUnswitch has a size-limitation on the loop it handles, which is not really true: If a no-side-effects loop is created and the intent is to later delete it, it should not be limited by the size heuristic.

That is true, but the size of the overall loop is probably not too big in practice, if there is a no-op part. If we implement your suggestion below (to jump to the smaller one for the invariant case in the larger one), the size increase should be negligible. It would be interesting to check how many dead loops get missed by unswitching. If there are enough cases, it should be easy to share the detection logic between both passes and also use it in LoopDeletion.

> On the other hand, I wonder if the idea here is to only isolate totally dead paths per this first draft or perhaps also paths where specifically the condition is invariant? This would be the advantage I can see compared to D93734 <https://reviews.llvm.org/D93734>: to increase unswitching where there may be side-effects that is known not to alias with the ToDuplicate instructions.

Yes, I think the main advantage of the 'partial' unswitching is that neither path needs to be dead, it just cannot clobber the condition we unswitch on. So for example, reductions would be fine or stores to locations that do not alias any loads feeding the condition. The original version of the patch did not yet implement that. But I updated the patch to use MemorySSA to also catch this case.

> What about the original loop: If it is entered (instead of the new smaller loop your patch creates), it may still be true that the path where the condition becomes invariant is entered. So (at least theoretically) the full loop should in that case branch into the smaller loop. This would also eliminate the need to duplicate instructions in the preheader.

Yes, in some cases it would be possible to just jump to the smaller loop. Unfortunately this will require some extra checks and work to set up the continuation values in the smaller loop. This is certainly something interesting worth exploring, but probably not in the initial version.

> If we only want to achieve the "early exit" result, in other words that this patch will create a new loop with no side-effects (and then depend on later passes to remove it), it seems simpler to me to have this as a separate pass (or perhaps as a fix-up in LoopDeletion per D93734 <https://reviews.llvm.org/D93734>).

The aim for this patch is to catch more cases in general, including cases that are not no-ops. The updated version supports memory ops that do not alias the loads feeding the condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764

Files:
  llvm/lib/Transforms/Scalar/LoopUnswitch.cpp
  llvm/test/Transforms/LoopUnswitch/partial-unswitch.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D93764.313906.patch
Type: text/x-patch
Size: 32726 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201228/0b866473/attachment.bin>