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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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;
----------------
void wrote:
> 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.
Is this really better looking?
```
diff --git a/llvm/lib/CodeGen/CallBrPrepare.cpp b/llvm/lib/CodeGen/CallBrPrepare.cpp
index 9835b5b41e2f..752e2d6f442f 100644
--- a/llvm/lib/CodeGen/CallBrPrepare.cpp
+++ b/llvm/lib/CodeGen/CallBrPrepare.cpp
@@ -117,8 +117,8 @@ bool CallBrPrepare::SplitCriticalEdges(ArrayRef<CallBrInst *> CBRs,
     for (unsigned i : UniqueCriticalSuccessorIndices) {
       BasicBlock *Synth = SplitKnownCriticalEdge(CBR, i, Options);
       assert(Synth && "Failed to split a known critical edge from callbr");
-      if (Synth)
-        Changed = true;
+      (void)Synth;
+      Changed = true;
     }
   }
   return Changed;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872