[PATCH] D139872: [llvm][CallBrPrepare] split critical edges

[Bill Wendling via Phabricator via llvm-commits]

void added inline comments.


================
Comment at: llvm/lib/CodeGen/CallBrPrepare.cpp:114
+      assert(Synth && "Failed to split a known critical edge from callbr");
+      if (Synth)
+        Changed = true;
----------------
nickdesaulniers wrote:
> aeubanks wrote:
> > nickdesaulniers wrote:
> > > aeubanks wrote:
> > > > unnecessary if
> > > `SplitKnownCriticalEdge` is fallible.  If we don't end up splitting the critical edge, then we shouldn't mark denote that the pass has made changes, right?
> > there's an assert right above that `Synth != nullptr`, it doesn't make sense to assert on something then check if it's true
> > 
> > looking at `SplitKnownCriticalEdge`, it only returns `nullptr` for the options we're passing it when `DestBB->isEHPad()`, can that happen?
> > there's an assert right above that Synth != nullptr, it doesn't make sense to assert on something then check if it's true
> 
> If assertions are disabled, we should still denote correctly whether the pass made modifications.
> 
> > looking at SplitKnownCriticalEdge, it only returns nullptr for the options we're passing it when DestBB->isEHPad(), can that happen?
> 
> If someone was deranged enough to mix C++ structured exception handling with asm goto, perhaps.
> 
> ```
> int y;
> asm goto ("":"=r"(y):::out);
> try {} catch (int x) {
> out:
> }
> ```
> Though looks like the frontend rejects it: https://godbolt.org/z/dKGnjKK49.
> 
> I've also tried:
> ```
> define i32 @x() {
>   %out = callbr i32 asm "", "=r,!i"() to label %direct [label %lp]
> direct:
>   br label %lp
> lp:
>   %foo = landingpad { ptr, i32 } cleanup
>   ret i32 42
> }
> ```
> which fails the verifier check
> ```
> Block containing LandingPadInst must be jumped to only by the unwind edge of an invoke.
>   %foo = landingpad { ptr, i32 }
>           cleanup
> LandingPadInst needs to be in a function with a personality.
>   %foo = landingpad { ptr, i32 }
>           cleanup
> ```
> I don't know enough about the rest of the EHPad instructions to know if someone could conjure up such an abomination, hence extra checks. "Let's support those if we actually ever do see them in the wild."
I agree with @aeubanks. This sequence looks bad. I know that asserts aren't enabled for a release, but it's either bad or it isn't. If it's bad, we should at the very least report it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872