[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:
> 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;
> ```
That's how it's done in other places. E.g. in `llvm/include/llvm/CodeGen/LiveVariables.h`:

```
  /// removeVirtualRegisterKilled - Remove the specified kill of the virtual
  /// register from the live variable information. Returns true if the
  /// variable was marked as killed by the specified instruction,
  /// false otherwise.
  bool removeVirtualRegisterKilled(Register Reg, MachineInstr &MI) {
    if (!getVarInfo(Reg).removeKill(MI))
      return false;

    bool Removed = false;
    for (MachineOperand &MO : MI.operands()) {
      if (MO.isReg() && MO.isKill() && MO.getReg() == Reg) {
        MO.setIsKill(false);
        Removed = true;
        break;
      }
    }

    assert(Removed && "Register is not used by this instruction!");
    (void)Removed;
    return true;
  }
```

It's not particularly satisfying, but at least it appears to be consistent with the rest of the code base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139872