[PATCH] D81607: BreakCriticalEdges for callbr indirect dests

Thu Jun 11 09:53:59 PDT 2020

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a subscriber: efriedma.
nickdesaulniers added a comment.

@nathanchance ran significant testing on this to ensure this patch doesn't regress linux ToT "mainline" or 5.4 stable kernels.

https://del.dog/raw/ocyphupiph

https://del.dog/raw/mollilekad

================
Comment at: llvm/test/Transforms/CallSiteSplitting/callsite-split-callbr.ll:39
+  call void @callee(i1 %phi)
+  callbr void asm sideeffect "", "r,X,~{dirflag},~{fpsr},~{flags}"(i1 %phi, i8* blockaddress(@caller, %End2))
+  to label %End [label %End2]
----------------
fhahn wrote:
> Does `callbr` always take the destinations as `blockaddress` or could an arbitrary `i8*` value be used? Then doing the splitting here would only be safe if we can update the `blockaddress` constant and not in other cases I think.
Yes; always `blockaddress`es.  @efriedma and I have discussed removing the duplication in `callbr`; the indirect destination list (`[label %End2]`) duplicates the arguments to the inline asm (`blockaddress(@caller, %End2`).

This is an invariant that is checked by IR verification.

(Using grammar from https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html)

But the duplication is brittle because while `setSuccessor` does work correctly, the lower level `setOperand` does not update `callbr` operands correctly.  It's also hard to distinguish between the labels passed in for `asm goto` vs extended inline asm inputs that are labels, ex `foo: asm goto (""::"r"(foo)::bar); bar:;` (we know `bar` is a potential destination, but provide no guarantees that `foo` can be safely jumped to from the inline asm. (InputOperands vs GotoLabels)

Removing the duplication should fix that brittleness, but it's also orthogonal to this patch.  Only `blockaddress`es can be indirect destinations that we preserve.  Arbitrary addresses can be arguments to inline asm (InputOperands), but we provide no guarantees unless the label in in the final inline asm parameter list (GotoLabels), as opposed to the list of inputs (InputOperands).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81607