- User Since
- Mar 18 2015, 8:30 AM (239 w, 2 h)
May 11 2018
LGTM, with these few nits, thanks!
May 10 2018
Incoming offset/register values of each successor block should match outgoing offset/register values of its predecessors.
May 7 2018
Apr 20 2018
@thegameg Hm, that's strange. They pass on my side. What are the failures that you're having?
Apr 19 2018
Mar 20 2018
Feb 21 2018
Feb 13 2018
Nov 9 2017
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) ?
Nov 1 2017
Rebased the patch.
Thank you all for the comments!
Oct 19 2017
Removed MachineInstr::isDirective(). Added countsAsInstruction(const MachineInstr &MI) in BranchFolding. Replaced isDirective() with isMetaInstruction() in TailDuplication.
Oct 18 2017
Could you try changing it to not add MachineInstr::isDirective()?
Oct 17 2017
Is there anything else that should be done for this patch?
Oct 10 2017
Oct 6 2017
Oct 5 2017
Addressed comments. Added changes for tests that were previously failing.
Oct 3 2017
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.
Aug 17 2017
Aug 4 2017
Here are a few more details, on top of what Petar said:
Aug 3 2017
Aug 1 2017
Removed unnecessary call to initializeCFIInfo() in X86CallFrameOptimization.cpp.
Using SmallPtrSet instead of std::set in MachineBasicBlock.cpp.
Jul 31 2017
Do you plan on adding MIR support?
Jul 25 2017
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.
Jul 5 2017
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.
Jun 28 2017
Rebased patch again.
Jun 27 2017
Rebased the patch.
Jun 23 2017
Jun 22 2017
Addressed other comments.
Changed the code in updateCFIInfoSucc() a bit since AdjustCFAOffset is no longer used.
Jun 19 2017
Updated a comment in ComputeCommonTailLength() in BranchFolding.cpp.
Jun 15 2017
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).
Jun 7 2017
Addressed comments about hiding CFI algorithms from the existing passes.
Jun 2 2017
Here is an implementation of the approach that was suggested.
Mar 31 2017
They currently affect code generation. This is my biggest complaint. I don't see why they should affect these passes.
Mar 28 2017
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?
Mar 16 2017
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?
Mar 15 2017
Added missing CHECKs in load-store-left-right.ll test.
Mar 14 2017
Mar 13 2017
Mar 9 2017
Addressed comments. Thank you for the review.
Mar 1 2017
Uploaded the patch with more context.
Feb 28 2017
Jul 21 2016
I would start working on implementing this if no one has anything against the proposed solution.
Jul 12 2016
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.
Jun 29 2016
Does anyone have any comments?
Jun 8 2016
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:
May 13 2016
May 10 2016
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?
Apr 28 2016
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.
Apr 11 2016
Thank you for the review.
I addressed your comments in the patch and answered your questions inline.
Mar 17 2016
Thank you for the review, Michael!
Mar 10 2016
Nov 4 2015
Made the following changes according to the D13767 that landed:
- Removed the changes made in the AsmPrinterDwarf.cpp (adding support for OpAdjustCfaOffset)
- Changed the condition from hasDebugInfo() to usePreciseUnwindInfo() in X86MCInstLower.cpp.
Oct 26 2015
Removed unnecessary calls to createAdjustCfaOffset method.
Oct 23 2015
Removed adding cfi instructions to FrameInstructions vector.
Mar 25 2015