This is an archive of the discontinued LLVM Phabricator instance.

Fix interaction of CFI instructions with MachineOutliner.
ClosedPublic

Authored by efriedma on Jun 2 2022, 3:50 PM.

Details

Summary

Fix interaction of CFI instructions with MachineOutliner.

  1. When checking if a candidate contains a CFI instruction, actually iterate over all of the instructions, instead of stopping halfway through.
  2. Make sure copied CFI instructions refer to the correct instruction.

Fixes https://github.com/llvm/llvm-project/issues/55842

Diff Detail

Unit TestsFailed

Event Timeline

efriedma created this revision.Jun 2 2022, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 3:50 PM
efriedma requested review of this revision.Jun 2 2022, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 3:50 PM
efriedma retitled this revision from Fix interaction of CFI with MachineOutliner. to Fix interaction of CFI instructions with MachineOutliner..Jun 2 2022, 3:51 PM
efriedma edited the summary of this revision. (Show Details)

Thank you!

Is it possible or worthwhile to have a test case?

efriedma planned changes to this revision.Jun 2 2022, 4:40 PM

Should be possible to add a testcase, I think. Testcases are a bit tricky to write, though.

Need to think about the MachineOutliner.cpp change a bit more; it's broken.

I guess if we're tail-calling, and we grab all the CFI instructions, it should be okay to just clone all of them? But that's making me doubt the way CFI instructions are being counted; I don't think it's safe to say that getFrameInstructions().size() is actually the number of CFI instructions in the function. It is if you're writing MIR testcases for MachineOutliner, because of the way serialization works, but I don't think that works in general.

If we fix that check, and fix MachineOutliner.cpp to properly clone the instructions, then I guess everything works? But I'd have to spend a day writing testcases.

efriedma updated this revision to Diff 433932.Jun 2 2022, 6:11 PM
efriedma edited the summary of this revision. (Show Details)

Updated according to my best understanding of how this is supposed to work. Not really confident I'm actually getting this right, but seems like an improvement. Modified some tests to cover the new changes.

kyulee added a subscriber: kyulee.Jun 4 2022, 8:54 AM
kyulee added inline comments.
llvm/lib/CodeGen/MachineOutliner.cpp
674–680

I think it's somewhat inefficient by cloning the entire instruction, removing this and then adding one back.
CFIIndex should be reassigned for the new function (because the old function might optimize or shuffle it), so we will create new CFI instruction while adding CFIIndex to the new function. Can we just do this way?

// Don't keep debug information for outlined instructions.
auto DL = DebugLoc();
if (I->isCFIInstruction()) {
  unsigned CFIIndex = I->getOperand(0).getCFIIndex();
  MCCFIInstruction CFI = Instrs[CFIIndex];
  BuildMI(MBB, MBB.end(), DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
       .addCFIIndex(MF.addFrameInst(CFI));
} else {
  MachineInstr *NewMI = MF.CloneMachineInstr(&*I);
  NewMI->dropMemRefs(MF);
  NewMI->setDebugLoc(DL);
  MBB.insert(MBB.end(), NewMI);
}
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
7016

I think this is a nice catch. We should check the entire instructions of the candidate which will be outlined, instead of looking the range of index to the hashes.

7030

I'm just a bit confused about this code. Are you checking a function with a single block?
Even that, I don't think walking all the instruction in the block adds value -- do you see more outlining or improvement by doing this?
The existing logic that just checks the size of CFIInstructions might be enough conservatively although it might include dead CFIs.
If you see the following logic later, the machine outliner actually outlines a tail case only if CFIs are outlined.

efriedma added inline comments.Jun 6 2022, 9:40 AM
llvm/lib/CodeGen/MachineOutliner.cpp
674–680

The efficiency difference is unlikely to matter, but sure, this is probably more clear.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
7030

Probably the size of CFIInstructions is greater than or equal to the number of CFI instructions in the function (unless we clone a CFI instruction, which probably shouldn't happen).

That said, I don't really want to trust the size of CFIInstructions. Any heuristic that depends on it will be very confusing to anyone looking at MIR dumps (since we print CFI instructions inline).

The single basic block restriction isn't strictly necessary; I'll try to come up with a better way to write this.

kyulee added inline comments.Jun 6 2022, 9:58 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
7030

Probably the size of CFIInstructions is greater than or equal to the number of CFI instructions in the function

I think this is conservatively safe to use it in this context if that happens -- we're likely to bail-out outlining due to mis-matches in # of CFI instructions. I wonder how many cases we have, in practice.

Any heuristic that depends on it will be very confusing to anyone looking at MIR dumps (since we print CFI instructions inline).

Can you elaborate it? Probably I don't understand the problem clearly. Let say we have N as CFIInstructions' size while there are actually M CFI instructions in the function body (M <= N). Didn't M instances still have valid CFI Index, assigned to the function? Do you have an example to show this is the problem?

efriedma added inline comments.Jun 6 2022, 10:47 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
7030

See the changed llvm/test/CodeGen/AArch64/machine-outliner-cfi-tail.mir ; without this change, we don't transform.

kyulee added inline comments.Jun 6 2022, 11:24 AM
llvm/test/CodeGen/AArch64/machine-outliner-cfi-tail.mir
37 ↗(On Diff #433932)

I understand this will make a difference to outline it if we can prove this is dead and thus safely ignore it -- probably at the cost of walking IRs or blocks for all parent functions of candidates.
Do you see this case often in practice or is it just synthesized for this testing purpose?
Even if that is the case, I think this is irrelevant for this fix, and probably suggest another patch as an optimization (or improving it).

efriedma updated this revision to Diff 434981.Jun 7 2022, 4:01 PM
efriedma edited the summary of this revision. (Show Details)

Address review comments.

I don't have any evidence the CFIInstructions.size() thing can cause a miscompile, so leaving out for now.

kyulee added a comment.Jun 7 2022, 4:05 PM

Thanks for fixing this!
LGTM.

kyulee accepted this revision.Jun 7 2022, 4:06 PM
This revision is now accepted and ready to land.Jun 7 2022, 4:06 PM
smeenai accepted this revision.Jun 7 2022, 4:26 PM

LGTM, thank you!

llvm/lib/CodeGen/MachineVerifier.cpp
2216

Unfortunately, this doesn't appear to be enough to catch this issue. If I undo the other changes in this diff and remove the assertion I added in D126919, the test case from https://github.com/llvm/llvm-project/issues/55842 still crashes with an unexpected instruction error instead of hitting this check.

smeenai added inline comments.Jun 7 2022, 4:38 PM
llvm/lib/CodeGen/MachineVerifier.cpp
2216

NVM, this was PEBKAC – I forgot to pass -verify-machineinstrs to llc. I imagine Clang does that automatically.

efriedma added inline comments.Jun 7 2022, 5:11 PM
llvm/lib/CodeGen/MachineVerifier.cpp
2216

verify-machineinstrs is expensive because it checks after every pass. So it's off by default in all tools.

This revision was landed with ongoing or failed builds.Jun 10 2022, 1:38 PM
This revision was automatically updated to reflect the committed changes.