Page MenuHomePhabricator

[MachineScheduler] Reorder cfi to avoid PostRA scheduling differences
Needs ReviewPublic

Authored by yechunliang on Dec 11 2019, 6:02 AM.

Details

Summary

As cfi_instruction is defined as sched boundary, it will split ScheduleRegion, this will make CodeGen different with and without cfi_instructions (with/without --strip-debug). Moving cfi_instruction to RegionEnd and set it as RegionEnd to avoid breaking scheduler Regions.

Diff Detail

Event Timeline

yechunliang created this revision.Dec 11 2019, 6:02 AM

As far as I understand it I don't think it is safe to move CFI_INSTRUCTIONs
around like this. CFI_INSTRUCTIONs describe the effect of specific instructions.
We encode where this 'description' starts by the position of the CFI_INSTRUCTION
in the instruction stream. So, if you move the CFI_INSTRUCTION like this you
are changing the meaning.

As I mention in the ticket [0], I think a good fix would involve removing all
CFI_INSTRUCTIONS before scheduling, and re-inserting them in the correct positions
afterwards. Unless I misunderstand, the "correct position" for a CFI_INSTRUCTION
is immediately after the instruction which it describes.

[0] https://bugs.llvm.org/show_bug.cgi?id=37240

As cfi_instruction is defined as sched boundary

so... should that be changed?

rnk added a subscriber: rnk.Dec 11 2019, 10:45 AM

+1 to what @Orlando said. I believe this is consistent with how epilogue CFI instructions were added, it was done late to avoid interfering with tail merging and other passes.

It is, however, easier said than done. Currently codegen works like this:

  • Do prologue/epilogue insertion, insert CFI precisely next to CSR saves and SP adjustments
  • Invariant: Later passes do not "mess" with prologue, but this is not well specified. The FrameSetup flags are one way we do this, this sched boundary marking seems like another.

A new model could be:

  • Do PEI, do not insert CFI, record some key info, like hasFP, did stackalign, hasBP, etc.
  • Invariant: Passes do not incorrectly reschedule instructions across prologue instrs (SP adjustments, CSR saves)
  • Invariant: Passes do not break late CFI insertion
  • Prior to, or concurrent with, AsmPrinter, reanalyze the prologue, insert CFI instructions precisely where necessary.

The challenge would be nailing down exactly what passes can and cannot do between PEI and AsmPrinter such that CFI can always be reliably inserted. Coping with multi-BB prologues from shrink wrapping is an example of such a challenge. This new design would also have the benefit that it would be truly asynchronous, meaning it is accurate at all instruction boundaries, not just after the prologue and around calls.

tellenbach edited reviewers, added: tellenbach; removed: dtell.Dec 11 2019, 2:40 PM
tellenbach added a subscriber: tellenbach.

+1 to all comments above.

I'd also prefer a solution that postpones the insertion of CFI instructions rather than changing scheduling. It should be okay to remember the overall frame-setup and insert the instructions at later stages (probably right before AsmPrinting). I've implemented a solution doing that for frame-lowering and it worked quite well. However, there are other passes that insert CFI instructions and I haven't finish it (yet).

Unless I misunderstand, the "correct position" for a CFI_INSTRUCTION
is immediately after the instruction which it describes.

This really depends on the particular CFI instruction but I guess .cfi_def_cfa_offset and .cfi_offset can be inserted after the last instruction marked with the frame-setup flag.

yechunliang added a comment.EditedDec 12 2019, 4:50 AM

Base on the solution "Postpone insertion of CFI instructions (pull out - reinsert)", with the patch, the CFI Instruction has been pulled out, but the problem is need to find the "correct position" to re-insert the CFI Instruction.

I've implemented a solution doing that for frame-lowering and it worked quite well. However, there are other passes that insert CFI instructions and I haven't finish it (yet).

@tellenbach, glad to hear you are working on this, maybe you have researched more than me, would you please continue to fix the issue? Thanks.