[MachineOutliner] Fix label outlining regression introduced in D125072

[duck-37]

Summary

Discovered by @zjaffal

Due to a change in the APIs used to determine what instructions can be outlined, the check for label outling was never hit. Instead, all labels were considered invisible, which is the opposite of the intended behavior and causes obscure crashes down the line. We now replicate the original behavior more closely, with explicit checks for known-good and known-bad instruction types.

[paquette]

Thanks for the fix, code looks good, but do you think you could write a MIR testcase instead of an IR testcase for this?

llvm/test/CodeGen/AArch64/machine-outliner-overlap.mir is a more recent test you could base it off of.

[duck-37]

Much better test.

[duck-37]

Fix diff.

[duck-37]

Fix diff, take two.

[paquette]

LGTM, thanks!

[duck-37]

@zjaffal can you confirm that this version of the patch also fixes your issue?

llvm/lib/CodeGen/TargetInstrInfo.cpp

outliner::InstrType TargetInstrInfo::getOutliningType(
   MachineInstr &MI = *MIT;
 
   // NOTE: MI.isMetaInstruction() will match CFI_INSTRUCTION, but some targets
   // have support for outlining those. Special-case that here.
   if (MI.isCFIInstruction())
     // Just go right to the target implementation.
     return getOutliningTypeImpl(MIT, Flags);
 
-  // Don't allow instructions that don't materialize to impact analysis.
-  if (MI.isMetaInstruction())
-    return outliner::InstrType::Invisible;
-
   // Be conservative about inline assembly.
   if (MI.isInlineAsm())
     return outliner::InstrType::Illegal;
 
   // Labels generally can't safely be outlined.
   if (MI.isLabel())
     return outliner::InstrType::Illegal;
 
+  // Don't let debug instructions impact analysis.
+  if (MI.isDebugInstr())
+    return outliner::InstrType::Invisible;
+
+  // Some other special cases.
+  switch (MI.getOpcode()) {
+    case TargetOpcode::IMPLICIT_DEF:
+    case TargetOpcode::KILL:
+    case TargetOpcode::LIFETIME_START:
+    case TargetOpcode::LIFETIME_END:
+      return outliner::InstrType::Invisible;
+    default:
+      break;
+  }
+
   // Is this a terminator for a basic block?
   if (MI.isTerminator()) {
     // If this is a branch to another block, we can't outline it.
     if (!MI.getParent()->succ_empty())
       return outliner::InstrType::Illegal;
 
     // Don't outline if the branch is not unconditional.
     if (isPredicated(MI))

llvm/test/CodeGen/AArch64/machine-outliner-labels.mir

+# RUN: llc -mtriple aarch64 -run-pass=machine-outliner -verify-machineinstrs %s -o - | FileCheck %s
+# CHECK-NOT: OUTLINED_FUNCTION
+
+...
+---
+name:            foo1
+tracksRegLiveness: true
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0:
+    liveins: $x0
+    $x0 = ADDXri $x0, 0, 0
+    EH_LABEL <mcsymbol .Ltmp0>
+    EH_LABEL <mcsymbol .Ltmp1>
+    EH_LABEL <mcsymbol .Ltmp2>
+    EH_LABEL <mcsymbol .Ltmp3>
+    RET_ReallyLR implicit $x0
+...
+---
+name:            foo2
+tracksRegLiveness: true
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0:
+    liveins: $x0
+    $x0 = ADDXri $x0, 0, 0
+    EH_LABEL <mcsymbol .Ltmp0>
+    EH_LABEL <mcsymbol .Ltmp1>
+    EH_LABEL <mcsymbol .Ltmp2>
+    EH_LABEL <mcsymbol .Ltmp3>
+    RET_ReallyLR implicit $x0
+...
+---
+name:            foo3
+tracksRegLiveness: true
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0:
+    liveins: $x0
+    $x0 = ADDXri $x0, 0, 0
+    EH_LABEL <mcsymbol .Ltmp0>
+    EH_LABEL <mcsymbol .Ltmp1>
+    EH_LABEL <mcsymbol .Ltmp2>
+    EH_LABEL <mcsymbol .Ltmp3>
+    RET_ReallyLR implicit $x0
+...