[PATCH] D32699: [PM/Unswitch] Teach the new simple loop unswitch to handle loop invariant PHI inputs and to rewrite PHI nodes during the actual unswitching.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 6 inline comments as done.
chandlerc added a comment.

Thanks both for the review. Landing with some fixes, but further discussion below and I'll follow-up with anything else.

In https://reviews.llvm.org/D32699#748898, @davide wrote:

> I'd really appreciate if @dberlin could take a look at the rewriter (even post-commit).


Me too! (But I think this can be post-commit too.)



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:165
+/// terminator.
+void rewritePHINodesForUnswitchedExitBlock(BasicBlock &UnswitchedBB,
+                                BasicBlock &ExitingBB,
----------------
sanjoy wrote:
> I'd have used a slightly different interface.  Instead of `rewritePHINodesForUnswitchedExitBlock` and `rewritePHINodesForExitAndUnswitchedBlocks`, I'd have a single function `rewriteExitPHIs` with the signature of `rewritePHINodesForExitAndUnswitchedBlocks` that did different things based on whether `ExitBB` and `UnswitchedBB` were equal.  Right now the PHI handling logic is effectively split between the caller (that decides whether we need a split PHI) and the callee `rewritePHINodesForUnswitchedExitBlock` or `rewritePHINodesForExitAndUnswitchedBlocks` that does the rewrite.
> 
It's funny, I had that exact thing originally.

I removed it for two reasons. One, it made the code here (IMO) substantially harder to read than either of the ones here because you end up with essentially two independent halves of the loop body for the two cases. This made the commenting and other aspects pretty confusing IMO.

Second, some of the callers already had distinguished between these two cases and it seemed nice to not revisit that decision, but instead make the precise rewrite locally where we have already tested this.

I also think that this could be improved by having a dedicated `ExitBB->phis()` method that produces the requisite range. I'll look at adding that. Then this whole thing becomes much cleaner IMO.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:217
+    // required to produce identical results.
+    for (int64_t i = PN->getNumIncomingValues() - 1; i >= 0; --i) {
+      if (PN->getIncomingBlock(i) != &ExitingBB)
----------------
sanjoy wrote:
> Why `int64_t` ?
I have no idea. Switched to int.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:491
+    if (pred_empty(ExitBB)) {
+      // Only rewrite once.
+      if (UnswitchedExitBBs.insert(ExitBB).second)
----------------
sanjoy wrote:
> Instead of adding a new `UnswitchedExitBBs`, I'd re-use `SplitExitBBMap` for this -- for unswitched block `BB` that doesn't need splitting, I'd have `SplitExitBBMap[BB] = BB`.  For instance, if you go with the unified `rewriteExitPHIs` routine above, the the code will look like:
> 
> ```
>     if (!SplitExitBB) {
>       // If this is the first time we see this, do the split and remember it.
>       SplitExitBB = pred_empty(ExitBB) ? ExitBB : SplitBlock(ExitBB, &ExitBB->front(), &DT, &LI);
>       rewriteExitPHIs(*ExitBB, *SplitExitBB, *ParentBB, *OldPH);
>       updateLoopExitIDom(ExitBB, L, DT);
>     }
> ```
> 
> with an appropriate relaxation in `updateLoopExitIDom`.
IMO, this is somewhat tied up in the other issue.

I actually wrote this version as well before writing the one you see here. The thing that bugged me about it was that splitting the exit blocks doesn't seem likely to be the common case. And so it seems weird to end up with a map that will often just be a set.

That, plus, we have a local and easy way to distinguish between the two and so it seemed like we should keep the data structures separate.

However, it does result in a lot more code. More efficient code, but more code all the same.

I'm somewhat divided on this. If my explanation convinces you to like this approach, I'll keep it. But if after hearing why I wasn't fond of the approach you still think its worth simplifying, let me know, and I'll do a follow-up patch to simplify everything.


https://reviews.llvm.org/D32699