[GlobalISel] Add a new G_INVOKE_REGION_START instruction to fix an EH bug.
authorAmara Emerson <amara@apple.com>
Sun, 13 Nov 2022 09:43:04 +0000 (01:43 -0800)
committerAmara Emerson <amara@apple.com>
Wed, 7 Dec 2022 18:28:51 +0000 (10:28 -0800)
commit53445f5b1cfcbd50fc0fb1f50a1724a7d6f092f8
tree1c76469e3512fdf276c7897e3869a062a9e9efef
parent14ea545a7d16dad1d26c8ba7f817a1b3d33d4132
[GlobalISel] Add a new G_INVOKE_REGION_START instruction to fix an EH bug.

We currently have a bug where the legalizer, when dealing with phi operands,
may create instructions in the phi's incoming blocks at points which are effectively
dead due to a possible exception throw.

Say we have:

throwbb:
  EH_LABEL
  x0 = %callarg1
  BL @may_throw_call
  EH_LABEL
  B returnbb

bb:
  %v = phi i1 %true, throwbb, %false....

When legalizing we may need to widen the i1 %true value, and to do that we need
to create new extension instructions in the incoming block. Our insertion point
currently is the MBB::getFirstTerminator() which puts the IP before the unconditional
branch terminator in throwbb. These extensions may never be executed if the call
throws, and therefore we need to emit them before the call (but not too early, since
our new instruction may need values defined within throwbb as well).

throwbb:
  EH_LABEL
  x0 = %callarg1
  BL @may_throw_call
  EH_LABEL
  %true = G_CONSTANT i32 1 ; <<<-- ruh'roh, this never executes if may_throw_call() throws!
  B returnbb

bb:
  %v = phi i32 %true, throwbb, %false....

To fix this, I've added two new instructions. The main idea is that G_INVOKE_REGION_START
is a terminator, which tries to model the fact that in the IR, the original invoke inst
is actually a terminator as well. By using that as the new insertion point, we
make sure to place new instructions on always executing paths.

Unfortunately we still need to make the legalizer use a new insertion point API
that I've added, since the existing `getFirstTerminator()` method does a reverse
walk up the block, and any non-terminator instructions cause it to bail out. To
avoid impacting compile time for all `getFirstTerminator()` uses, I've added a new
method that does a forward walk instead.

Differential Revision: https://reviews.llvm.org/D137905
15 files changed:
llvm/docs/GlobalISel/GenericOpcode.rst
llvm/include/llvm/CodeGen/MachineBasicBlock.h
llvm/include/llvm/Support/TargetOpcodes.def
llvm/include/llvm/Target/GenericOpcodes.td
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
llvm/lib/CodeGen/MachineBasicBlock.cpp
llvm/lib/CodeGen/MachineVerifier.cpp
llvm/test/CodeGen/AArch64/GlobalISel/invoke-region.ll [new file with mode: 0644]
llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-exceptions.ll
llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-unwind-inline-asm.ll
llvm/test/CodeGen/AArch64/GlobalISel/legalize-exceptions.ll
llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir
llvm/test/MachineVerifier/test_g_invoke_region_start.mir [new file with mode: 0644]