[PATCH] D127960: [InstCombine] Push freeze through recurrence phi

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D127960#3590870 <https://reviews.llvm.org/D127960#3590870>, @aqjune wrote:

>> I suspect that this also obsoletes the CanonicalizeFreezeInLoops pass, and we can probably drop it.
>
> ATM CanonicalizeFreezeInLoops targets patterns that are weakly related but different from this patch:
> (1) It deals with freeze(add nsw (phi, 1)). (foldFreezeIntoRecurrence is called only for freeze(phi) in this patch)

InstCombine handles this case as well: pushFreezeToPreventPoisonFromPropagating() will convert freeze(add nsw (phi, 1)) into add(freeze(phi), 1), and from there we will push the freeze out of the loop.

> (2) Freeze instructions that are not finally fed into the branch condition are hoisted as well - this is to allow successful SCEV analysis for all variables inside a loop

This code doesn't depend on the branch condition either, so I //think// it should be fine. I'll run the CanonicalizeFreezeInLoops tests through InstCombine to check whether we're missing any cases.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3803
+  if (StartBB->getFirstInsertionPt() == StartBB->end())
+    return nullptr; // Cannot insert into this block.
+
----------------
aqjune wrote:
> nlopes wrote:
> > aqjune wrote:
> > > Not sure whether this can happen; when does this condition meet?
> > I was also wondering about that, but I guess it may happen with malformed BBs (e.g., with just PHIs but no terminator). Unreachable BBs are allowed to be malformed in LLVM IR (which is a bit unfortunate).
> Oh okay, it makes sense to me.
Unreachable code still requires basic blocks to have terminators. What this was handling is a special case where no instructions can be inserted into a block for the case where an EH pad is also a terminator. This happens with `catchswitch` block pads, which can only contain phis and catchswitch. However, after trying to construct a test case for this, I don't think this is relevant for this code, because the successor of catchswitch can't be a loop (it would be another EH pad). So I've dropped this.

Instead I've added a check for invoke terminators, which we do need to handle (`@fold_phi_invoke_start_value`).


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

https://reviews.llvm.org/D127960