Page MenuHomePhabricator

violetav (Violeta Vukobrat)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 18 2015, 8:30 AM (230 w, 6 d)

Recent Activity

May 11 2018

violetav accepted D46671: Use iteration instead of recursion in CFIInserter.

LGTM, with these few nits, thanks!

May 11 2018, 7:33 AM
violetav added inline comments to D46671: Use iteration instead of recursion in CFIInserter.
May 11 2018, 7:01 AM

May 10 2018

violetav added a comment to D46671: Use iteration instead of recursion in CFIInserter.

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

May 10 2018, 8:03 AM

May 7 2018

violetav accepted D46444: Add option -verify-cfiinstrs to run verifier in CFIInstrInserter.

LGTM

May 7 2018, 6:33 AM
violetav accepted D46399: Skip unreachable blocks for CFIInstrInserter verify.

LGTM

May 7 2018, 4:49 AM

Apr 20 2018

violetav added a comment to D42848: Correct dwarf unwind information in function epilogue.

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

Apr 20 2018, 9:08 AM
violetav added inline comments to D42848: Correct dwarf unwind information in function epilogue.
Apr 20 2018, 8:15 AM

Apr 19 2018

violetav added inline comments to D42848: Correct dwarf unwind information in function epilogue.
Apr 19 2018, 9:10 AM

Mar 20 2018

violetav added inline comments to D42848: Correct dwarf unwind information in function epilogue.
Mar 20 2018, 9:46 AM

Feb 21 2018

violetav added inline comments to D42848: Correct dwarf unwind information in function epilogue.
Feb 21 2018, 7:46 AM

Feb 13 2018

violetav added a comment to D42848: Correct dwarf unwind information in function epilogue.

ping

Feb 13 2018, 5:20 AM

Nov 9 2017

violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

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) ?

Nov 9 2017, 8:55 AM

Nov 1 2017

violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

Rebased the patch.
Thank you all for the comments!

Nov 1 2017, 8:13 AM

Oct 19 2017

violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

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

Oct 19 2017, 9:25 AM

Oct 18 2017

violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

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

Oct 18 2017, 8:28 AM

Oct 17 2017

violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

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

Oct 17 2017, 7:36 AM

Oct 10 2017

violetav added inline comments to D35844: Correct dwarf unwind information in function epilogue.
Oct 10 2017, 10:13 AM

Oct 6 2017

violetav added inline comments to D35844: Correct dwarf unwind information in function epilogue.
Oct 6 2017, 9:02 AM
violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

Addressed comments.

Oct 6 2017, 8:58 AM

Oct 5 2017

violetav added inline comments to D35844: Correct dwarf unwind information in function epilogue.
Oct 5 2017, 9:30 AM
violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

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

Oct 5 2017, 9:20 AM

Oct 3 2017

violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

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.
Oct 3 2017, 8:47 AM

Aug 17 2017

violetav added inline comments to D35844: Correct dwarf unwind information in function epilogue.
Aug 17 2017, 8:39 AM

Aug 4 2017

violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

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

Aug 4 2017, 8:12 AM

Aug 3 2017

violetav updated subscribers of D35844: Correct dwarf unwind information in function epilogue.
Aug 3 2017, 12:51 AM

Aug 1 2017

violetav updated the diff for D35844: Correct dwarf unwind information in function epilogue.

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

Aug 1 2017, 4:49 AM

Jul 31 2017

violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

Do you plan on adding MIR support?

Jul 31 2017, 8:32 AM

Jul 25 2017

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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

Jul 25 2017, 9:24 AM
violetav added a comment to D35844: Correct dwarf unwind information in function epilogue.

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 25 2017, 9:20 AM
violetav created D35844: Correct dwarf unwind information in function epilogue.
Jul 25 2017, 9:18 AM

Jul 5 2017

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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.

Jul 5 2017, 5:19 AM

Jun 28 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

Rebased patch again.

Jun 28 2017, 2:45 AM

Jun 27 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

Rebased the patch.

Jun 27 2017, 10:47 AM

Jun 23 2017

violetav added inline comments to D18046: [X86] Providing correct unwind info in function epilogue.
Jun 23 2017, 8:39 AM

Jun 22 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

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

Jun 22 2017, 10:02 AM

Jun 19 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

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

Jun 19 2017, 6:11 AM

Jun 15 2017

violetav added inline comments to D18046: [X86] Providing correct unwind info in function epilogue.
Jun 15 2017, 6:31 AM
violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

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 15 2017, 6:27 AM

Jun 7 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

Addressed comments about hiding CFI algorithms from the existing passes.

Jun 7 2017, 12:29 PM

Jun 2 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

Here is an implementation of the approach that was suggested.

Jun 2 2017, 6:37 AM

Mar 31 2017

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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

Mar 31 2017, 8:37 AM

Mar 28 2017

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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 28 2017, 6:09 AM

Mar 16 2017

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

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 16 2017, 9:59 AM

Mar 15 2017

violetav updated the diff for D30464: [Mips] Add support to match more patterns for DEXT and CINS.

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

Mar 15 2017, 5:12 AM

Mar 14 2017

violetav updated the diff for D30464: [Mips] Add support to match more patterns for DEXT and CINS.

Rebased patch.

Mar 14 2017, 11:05 AM

Mar 13 2017

violetav updated the diff for D30464: [Mips] Add support to match more patterns for DEXT and CINS.

Addressed comments.

Mar 13 2017, 10:30 AM

Mar 9 2017

violetav added inline comments to D30464: [Mips] Add support to match more patterns for DEXT and CINS.
Mar 9 2017, 9:03 AM
violetav updated the diff for D30464: [Mips] Add support to match more patterns for DEXT and CINS.

Addressed comments. Thank you for the review.

Mar 9 2017, 9:02 AM

Mar 1 2017

violetav updated the diff for D30464: [Mips] Add support to match more patterns for DEXT and CINS.

Uploaded the patch with more context.

Mar 1 2017, 1:46 AM

Feb 28 2017

violetav created D30464: [Mips] Add support to match more patterns for DEXT and CINS.
Feb 28 2017, 8:55 AM

Jul 21 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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

Jul 21 2016, 9:07 AM

Jul 12 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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.

Jul 12 2016, 8:42 AM

Jun 29 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

Does anyone have any comments?

Jun 29 2016, 8:02 AM

Jun 8 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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:

Jun 8 2016, 5:15 AM

May 13 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

Hi Keno,

May 13 2016, 6:26 AM

May 10 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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?

May 10 2016, 3:17 AM

Apr 28 2016

violetav added a comment to D18046: [X86] Providing correct unwind info in function epilogue.

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 28 2016, 10:23 AM

Apr 11 2016

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

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

Apr 11 2016, 10:17 AM
violetav added inline comments to D18046: [X86] Providing correct unwind info in function epilogue.
Apr 11 2016, 10:08 AM

Mar 17 2016

violetav updated the diff for D18046: [X86] Providing correct unwind info in function epilogue.

Thank you for the review, Michael!

Mar 17 2016, 10:42 AM
violetav added inline comments to D18046: [X86] Providing correct unwind info in function epilogue.
Mar 17 2016, 10:29 AM

Mar 10 2016

violetav updated subscribers of D18046: [X86] Providing correct unwind info in function epilogue.
Mar 10 2016, 7:17 AM
violetav retitled D18046: [X86] Providing correct unwind info in function epilogue from to [X86] Providing correct unwind info in function epilogue.
Mar 10 2016, 7:14 AM

Nov 4 2015

violetav updated the diff for D14021: Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.

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.
Nov 4 2015, 5:41 AM

Oct 26 2015

violetav updated the diff for D14021: Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.

Removed unnecessary calls to createAdjustCfaOffset method.

Oct 26 2015, 10:17 AM

Oct 23 2015

violetav updated the diff for D14021: Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.

Removed adding cfi instructions to FrameInstructions vector.

Oct 23 2015, 10:39 AM
violetav retitled D14021: Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop from to Added cfi instructions for correct CFA calculation in case when movpc instruction expands to call and pop.
Oct 23 2015, 9:03 AM

Mar 25 2015

violetav added a comment to D8437: Added check for kind of UnqualifiedId in Declarator::isStaticMember().

ping

Mar 25 2015, 5:31 AM

Mar 19 2015

violetav retitled D8437: Added check for kind of UnqualifiedId in Declarator::isStaticMember() from to Added check for kind of UnqualifiedId in Declarator::isStaticMember().
Mar 19 2015, 7:01 AM