[PATCH] D81607: BreakCriticalEdges for callbr indirect dests

Tue Jun 16 14:51:16 PDT 2020

nickdesaulniers added a subscriber: eli.friedman.
nickdesaulniers added a comment.

In D81607#2091010 <https://reviews.llvm.org/D81607#2091010>, @nickdesaulniers wrote:

> In D81607#2090768 <https://reviews.llvm.org/D81607#2090768>, @efriedma wrote:
>
> > The reason we can't split edges coming out of indirectbr instructions is that in general, there is no way to fix the related blockaddresses in functions that contain multiple indirectbrs.  We can't rename the successor in one indirectbr without renaming it in all of them.  And if we rename it in all of them, the edge is still critical after the transform.
> >
> > We want to be sure the same issue doesn't apply to callbr instructions.  If it does, then we would need to fix the callers of SplitCriticalEdge, not SplitCriticalEdge itself.
>
>
> Wait, that sounds like a problem.  Imagine we have 3 `callbr`s, two that share the same indirect destination, and we decide to split one their critical edges. Then we update the `blockaddress` of that `callbr`, but not the others where the others' indirect branch now doesn't preserve the previous operations.  As if we added another `callbr` in the entry of this test case with an indirect destination of `%CallSiteBB`.

Ok, so I modified the added test case to do this, which also involves adding a case to the `phi` for the new incoming edge.  In that case, call site splitting does not kick in or perform any transformations.  Since call site splitting can't transform that, rewriting the `blockaddresses` is not an issue in this case.  Generally, that may be a problem, but I'd argue that's an issue with `blockaddresses` regardless of this patch.  It's currently on callers of `SplitCriticalEdge` not to miss updating `blockaddresses`, if there's more than one to worry about.  For instance, adding to the test case:

  @foo = global i8* blockaddress(@caller, %CallSiteBB)

call site splitting will modify `%CallSiteBB` such that the `call` to `@callee` is hoisted out into the newly split blocks.  If someone tried to use `@foo` for control flow, then `llvm::SplitCriticalEdge` would have made a transform that doesn't preserve previous control flow (is there a better term for that?).

(Going further, I'd be curious if having a `blockaddress` not scoped to a function is even generally useful?)

If we plan to radically change `callbr`, I think @fhahn 's suggestion <https://reviews.llvm.org/D81607#2090890> to simply disable call site splitting for `callbr` is likely sufficient (though that makes me think that we can still reach the failing assertion in `llvm::SplitEdge`, just no longer from call site splitting).  Or we could likely add a similar check I'm removing here to `llvm::SplitEdge` for indirect branches from `callbr`, then work through any issues that might cause in call site splitting.  I don't *need* call site splitting to work for `callbr`, I'm just trying to fix the currently failing assertion in `llvm::SplitEdge` that this test case reproduces, so I don't particularly care whether we take this form of the patch, or just disable this.

I plan on committing this tomorrow morning, as I'm trying to pin down regressions that D79794 <https://reviews.llvm.org/D79794> in current form will cause.  Please explicitly Accept or Reject the patch by then, and please feel empowered to revert otherwise.  Rereading the comments both @eli.friedman and @fhahn sound cautious, but no one has explicitly Rejected the patch, which leads me to hesitate on committing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81607