[PATCH] D138078: [CodeGenPrepare] split critical indirect edges from callbr w/ outputs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added reviewers: void, jyknight, efriedma, craig.topper.
Herald added subscribers: StephenFan, jdoerfert, pengfei, hiraditya.
Herald added a project: All.
nickdesaulniers requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Towards the goal of supporting outputs from callbr in its indirect
successors, split indirect edges that are critical when there are
outputs.

There are 2 invariants of LLVM that make this change necessary. Consider
a callbr with outputs that have physical register constraints, where the
indirect destination tries to use the result in a PHI.

The first constraint is that PHIs in MachineIR (MIR) do not support a
mix of physreg and virtreg operands. Physregs MUST be COPY'd to a
virtreg BEFORE they can be used in a PHI.

The second constraint is that PHIs MUST occur before non-PHIs in
MachineBasicBlocks. So where do we place the COPY, in the predecessor or
the successor? A: Neither. We split the "critical edge" that occurs when
the predecessor has multiple successors and the successor has multiple
predecessors.  In a follow up patch, we could then place the COPY in
that newly synthesized block, but this patch is just the critical edge
splitting.

This change was done prior to support for outputs along indirect edges,
because the changes to existing tests for outputs along the direct edge
are quite noisy and tricky to review itself.

Labels can be passed to asm goto in BOTH the input operands AND goto
labels lists, example:

  foo: asm goto ("# %0 %1"::"i"(&&foo)::foo);

This change has the implications that in the inline asm, the above
parameters need not compare equal; the goto label might refer to the
block synthesized when we split the critical edge while the input
operand will refer to the final destination. IMO, you're asking for
trouble if you have code doing this, but this patch will very much
change the codegen of LLVM in such an observable way. We initially
decided not to do this during the initial implementation of support for
outputs along "fallthrough" (aka "direct") edges from callbr, but
support for outputs along indirect edges is more valuable (subjective)
than labels passed simultaneously as input operands and goto labels
comparing equal. Also, this is how GCC chose to implement support for
outputs along indirect edges. Document this behavior change.

While CodeGenPrepare seems like a curious place to split critical edges,
the question "when is the best time to split critical edges" is perhaps
important to consider. SelectionDAG operates per BasicBlock and it's not
easy to visit other BasicBlocks during instruction selection.
MachineBasicBlock::canSplitCritical edge also avoids permitting critical
edge splitting for INLINEASM_BR indirect edges. The point of
CodeGenPrepare is to nudge LLVM IR to help SelectionDAG along. Also,
CodeGenPrepare has custom critical edge splitting for indirectbr, which
is a common theme for callbr.

Also, it might seem like we could also avoid splitting the
critical+indirect edge when there's no physreg defines, but it helps the
later patch for outputs along indirect edges not have to special case
virtreg COPYs vs physreg COPYs.

Link: https://github.com/llvm/llvm-project/issues/53562


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138078

Files:
  clang/docs/ReleaseNotes.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
  llvm/lib/CodeGen/CodeGenPrepare.cpp
  llvm/lib/Transforms/Utils/BreakCriticalEdges.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/Transforms/CodeGenPrepare/AArch64/callbr.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138078.475632.patch
Type: text/x-patch
Size: 15968 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221116/656130fc/attachment-0001.bin>