[PATCH] D105309: [InstCombine] Don't combine PHI before catchswitch

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

In D105309#2853898 <https://reviews.llvm.org/D105309#2853898>, @lebedev.ri wrote:

> This is seems like a too heavy hammer.
> `foldPHIArgZextsIntoPHI()` has:
>
>   // We cannot create a new instruction after the PHI if the terminator is an
>   // EHPad because there is no valid insertion point.
>   if (Instruction *TI = Phi.getParent()->getTerminator())
>     if (TI->isEHPad())
>       return nullptr;
>
> Is that the same check as here? (which means that the check here is incomplete?)

This check is fine, but as we can see here <https://github.com/llvm/llvm-project/blob/ce211c505b82e5bbb68b936968d9b54608285416/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp#L1306-L1307>, if the return value of `foldPHIArgZextsIntoPHI` is `nullptr`, it doesn't exit `visitPHINode` goes on to the next optimization within the function, so it couldn't have prevented the crash.

> If yes, then can you add a similar check to whatever fold is missing it, specifically?

I'll move the check to that specific part added in D98058 <https://reviews.llvm.org/D98058>.



================
Comment at: llvm/test/Transforms/InstCombine/catchswitch-phi.ll:1
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1"
+target triple = "wasm32-unknown-unknown"
----------------
rnk wrote:
> lebedev.ri wrote:
> > This isn't testing anything
> That's true, and I don't see any cast instructions to trigger the codepath we are looking at.
Oh sorry, I forgot to add the `RUN` line. Added now. Basically this shouldn't crash.
@rnk `phi` in `bb4` is the one that triggers the crash (without this patch).


================
Comment at: llvm/test/Transforms/InstCombine/catchswitch-phi.ll:33
+  ; itself.
+  %tmp3 = phi %struct.quux* [ %tmp1, %bb1 ], [ %tmp2, %bb2 ]
+  %tmp4 = catchswitch within none [label %bb5] unwind label %bb7
----------------
This is the instruction that triggers the crash without this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105309