LGTM, with these few nits, thanks!

Incoming offset/register values of each successor block should match outgoing offset/register values of its predecessors.

LGTM

LGTM

@thegameg Hm, that's strange. They pass on my side. What are the failures that you're having?

ping

Hi,
As this change was reverted in r317726, is the way to solve this issue to leave CFI_INSTRUCTION to be notDuplicable, and allow TailDuplication to generate different code when cfi_directives are present (e.g. not to duplicate epilogue blocks, if they contain cfi_directives) ?

Rebased the patch.
Thank you all for the comments!

Removed MachineInstr::isDirective(). Added countsAsInstruction(const MachineInstr &MI) in BranchFolding. Replaced isDirective() with isMetaInstruction() in TailDuplication.

Could you try changing it to not add MachineInstr::isDirective()?

Is there anything else that should be done for this patch?

Addressed comments.

Addressed comments. Added changes for tests that were previously failing.

Here is the new patch written based on the comments received and the previously implemented solution.
What is changed:

• Removed all changes from MachineBasicBlock.
• Removed changes related to updating in/out CFI information from common passes (BranchFolding.cpp and TailDuplicator.cpp).
• Merged CFIInfoVerifier and CFIInstrInserter into one pass that now calculates in/out CFI info for BBs, verifies it and inserts additional CFI instructions if needed. This pass isn't added in TargetPassConfig.cpp if the target does not maintain CFA info.

Hi Kyle,
Here are a few more details, on top of what Petar said:

Removed unnecessary call to initializeCFIInfo() in X86CallFrameOptimization.cpp.
Using SmallPtrSet instead of std::set in MachineBasicBlock.cpp.

Do you plan on adding MIR support?

@thegameg yes, I have just posted D35844 that fixes the issues.

This patch has been reviewed in D18046 and commited. It was reverted because it caused a number of sanitizer test failures on PPC64 buildbot, and also, it reported incorrect cfa offset values in recursive LLVM builds. These issues have been resolved in this patch.

Hi Francis,
I am not sure if I am missing something, but yes, it looks like you could do something similar for cfi_offset and cfi_restore; keep track of registers that should be saved/restored at the beginning and end of a basic block and then insert save/restore CFI instructions for appropriate registers if the basic blocks get reordered in a way that this information is no longer correct.
For the second question, I have started with something similar (inserted CFI instructions in epilogue and added additional CFI instrs that correct the CFA calculation rule both in the late pass), however, that was not preferable, because I was analyzing each BB, checking it for CFI instructions, and analyzing if additional instructions should be inserted, basically, I should have known this information already, and not need to do this analysis in the pass.

Rebased patch again.

Rebased the patch.

Removed AdjustCFAOffset.
Addressed other comments.
Changed the code in updateCFIInfoSucc() a bit since AdjustCFAOffset is no longer used.

Updated a comment in ComputeCommonTailLength() in BranchFolding.cpp.

Fixed a bug related to calculating values of incoming and outgoing cfa offset. For def_cfa and def_cfa_offset, negative offset value was used for calculation, and that was not the case for adjust_cfa_offset. Now, the values stored as in/out cfa offsets are the actual offsets set by cfi directives (and not their negative values).

Addressed comments about hiding CFI algorithms from the existing passes.

Here is an implementation of the approach that was suggested.

They currently affect code generation. This is my biggest complaint. I don't see why they should affect these passes.

Hi Kyle, thank you for commenting!
I am not sure that I fully understand what you are proposing.
Are you saying that we should attach info about incoming and outgoing cfa_offset and register to a MBB or actually add CFI instructions to the beginning and end of a MBB (and then remove unnecessary CFI instructions later)?
Do you think that CFI instructions in epilogue should be inserted during frame lowering, when instructions that change the SP are created? Same question for the store-to-push optimization. The problem is that CFI instructions that are inserted earlier (before MBB reordering, merging, splitting, that is done in the later passes (tail duplication, control flow optimizer)) affect code generation. If we add information about incoming and outgoing CFI offset and register to a MBB, we would definitely be more flexible. That would support multiple MBBs that change CFI offset (that have different CFI offset at their beginning and end) besides prologue and epilogue. However, the problem with CFI instructions impacting code generation would still be present, and would be handled similarly to the way it is handled in the proposed implementation. Do you have something else in mind for solving this issue?
What do you think is wrong with this approach?

Here is the current implementation of the proposed solution.
I have added a new flag to a MBB that represents its type based on its beginning and end cfi offset. There are 4 types of MBBs: PROLOGUE, EPILOGUE, IN_PATH and OUT_PATH. Default type of the MBB is IN_PATH. During prologue and epilogue emission in X86FrameLowering, corresponding MBBs are set as PROLOGUE and EPILOGUE, and also, MBBs that are not in the prologue/epilogue path are marked as OUT_PATH. There are changes in some of the passes that split or merge MBBs, where these types are updated correctly. There is also a late pass that inserts additional CFI instructions to the beginnings of the MBBs for the cases where epilogue is in the middle of the function and sets incorrect CFI offset and register values for those MBBs.
Does this look like a step in the right direction? Does anyone have any comments?

Added missing CHECKs in load-store-left-right.ll test.

Rebased patch.

Addressed comments.

Addressed comments. Thank you for the review.

Uploaded the patch with more context.

I would start working on implementing this if no one has anything against the proposed solution.

Hi Keno, thanks for commenting. As for the splitting of the epilogue over two MBBs, I have come across a case where 'pop' instructions were separated from the 'ret' instruction ('pop' instrs were in one MBB, and 'ret' was in another MBB). That case would be covered with the proposed solution. I haven't come across a case where 'add', 'pop' instructions that form an epilogue are split over multiple MBBs.

Does anyone have any comments?

Hi, I would like to propose a potential solution to this problem. If anyone knows a reason why this would fail, or if someone can think of a better way to do it, share your thoughts, help would be much appreciated. Here is the proposal:

Hi Keno,

To avoid having to calculate CFA on entry/end in the late pass, we could remember information about entry/end offset (and register) of a MBB in frame lowering and just use that info in the pass later. Possible problems could be BB merging, splitting and creating in later passes. Reid, is this what you had in mind? I am not sure if you meant to leave the insertion of CFI instructions in epilogue in the frame lowering, or to use the information available during frame lowering in order to simplify the implemented pass (but leave the insertion of CFI instructions in the late pass). Also, is storing information in side tables already used somewhere in the code?

The problem with TailDuplication is the main reason why a separate pass is needed. In shouldTailDuplicate() method, while going through instructions in the BB, the pass finds CFI instructions (that are marked as NotDuplicable in Target.td) and decides not to duplicate the given BB.

Hi Michael,
Thank you for the review.
I addressed your comments in the patch and answered your questions inline.

Thank you for the review, Michael!

Made the following changes according to the D13767 that landed:

1. Removed the changes made in the AsmPrinterDwarf.cpp (adding support for OpAdjustCfaOffset)
2. Changed the condition from hasDebugInfo() to usePreciseUnwindInfo() in X86MCInstLower.cpp.

Removed unnecessary calls to createAdjustCfaOffset method.

Removed adding cfi instructions to FrameInstructions vector.

ping