[RISCV] Do not outline CFI instructions when they are needed in EH

[• pcwang-thead]

Summary

We saw a failure caused by unwinding with incomplete CFIs, so we
can't outline CFI instructions when they are needed in EH.

[• pcwang-thead]

Rebase on precommit test D123364.

[luismarques]

We this change we now don't have any tests that show a difference compared with just if (MI.isPosition()) return outliner::InstrType::Illegal;
Please add a test that showcases that new, more fined grained, outlining criteria for position instructions.

[• pcwang-thead]

In D122634#3441112, @luismarques wrote:

We this change we now don't have any tests that show a difference compared with just if (MI.isPosition()) return outliner::InstrType::Illegal;
Please add a test that showcases that new, more fined grained, outlining criteria for position instructions.

Thanks for your advice, but what I changed in this patch is just the way to handle CFIs when unwinding is needed.
The original code is:

if (MI.isPosition()) {
    // We can manually strip out CFI instructions later.
    if (MI.isCFIInstruction())
      return outliner::InstrType::Invisible;
    return outliner::InstrType::Illegal;
}

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

[luismarques]

In D122634#3447432, @pcwang-thead wrote:

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

It's been a few days since my original comment so I may be misremembering the details but here's my perspective:

• Before your patch we had test coverage that showed the difference between a plain if (MI.isPosition()) return outliner::InstrType::Illegal; and the then-current implementation.
• With this patch we no longer have.
• With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.

Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)

[• pcwang-thead]

Rebase on new test.

[• pcwang-thead]

In D122634#3451886, @luismarques wrote:

In D122634#3447432, @pcwang-thead wrote:

These conditions existed since the first version of outlining implementation (D66210), so we won't see any differences of position instructions even if I added some tests in this patch.

It's been a few days since my original comment so I may be misremembering the details but here's my perspective:

• Before your patch we had test coverage that showed the difference between a plain if (MI.isPosition()) return outliner::InstrType::Illegal; and the then-current implementation.
• With this patch we no longer have.
• With this patch you are asserting that we want something more fine-grained than just "don't outline positions". Then let's have test coverage for that more fine-grained condition checking.

Also, in general, I'm in favor of the position that when you touch things you should take the opportunity to address closely related issues, within reason. That's always a tricky balance, and I don't know what the LLVM project policy is for that fine line, but it seems like something that we should strive for :)

Thank you for your explanation, I misunderstood what you meant :)

So I added some more fine-grained tests in D123364:

• machine-outliner-cfi.mir shows how we treat CFIs if there is no EH.
• machine-outliner-throw.ll shows how we treat CFIs if unwinding is needed.
• machine-outliner-position.mir shows that position instructions(which are labels) are illegal to outline.

Are there some other things that I should pay attention to?

[luismarques]

LGTM. Thanks!

[• pcwang-thead]

Rebase on fixed tests.

[• pcwang-thead]

Trigger buildbot.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

RISCVInstrInfo::getOutliningType(MachineBasicBlock::iterator &MBBI,
   MachineBasicBlock *MBB = MI.getParent();
   const TargetRegisterInfo *TRI =
       MBB->getParent()->getSubtarget().getRegisterInfo();
 
   // Positions generally can't safely be outlined.
   if (MI.isPosition()) {
     // We can manually strip out CFI instructions later.
     if (MI.isCFIInstruction())
-      return outliner::InstrType::Invisible;
+      // If current function has exception handling code, we can't outline &
+      // strip these CFI instructions since it may break .eh_frame section
+      // needed in unwinding.
+      return MI.getMF()->getFunction().needsUnwindTableEntry()
+                 ? outliner::InstrType::Illegal
+                 : outliner::InstrType::Invisible;
 
     return outliner::InstrType::Illegal;
   }
 
   // Don't trust the user to write safe inline assembly.
   if (MI.isInlineAsm())
     return outliner::InstrType::Illegal;