- User Since
- Jul 4 2017, 1:39 AM (103 w, 1 d)
Addressed most comments. The main changes are the following:
- added more cases to trivial-unswitch-profmd.ll
- brushed up basictest-profmd.ll
- added NewSI default case weight if the default successor was not an exit
Mon, Jun 24
I agree that those two checks seem to be inconsistent with each other, but I insist on my fix. It makes the method analyzeBlock() return same result (negative or positive) regardless of the flag ComputeFullInlineCost.
The root cause is that the analyzeBlock() returns different results (negative or positive) for different ComputeFullInlineCost. And the checks you mentioned if fixed could just hide this difference.
Fri, Jun 21
I still believe that we should separate a bug fix from introducing a new feature. As I read the comments this patch looks to be complex to be just a bugfix but not full enough to introduce a new and complete feature.
I could provide a one line fix with a test if I believe it is a complete fix for the bug. In other words I'm a bit stuck. Any suggestions are welcome.
Thu, Jun 20
Tue, Jun 18
Rebased to the latest HEAD. Please review. Its sibling patches have been landed.
Mon, Jun 17
Sun, Jun 16
Fri, Jun 14
- extracted a separate function GetFirstNegative()
- changed the test to check that the bug is fixed
In general I agree with this approach. But the bigger change it needs the stronger intention I have to fix the bug with a minimal change and to develop the feature as another separate patch. One issue bothers me: results of the InlineCost analyzer will depend on the flag ComputeFullInlineCost. Negative or positive decision will persist but the message and highest computed cost will be changing.
Tue, Jun 11
Sun, Jun 9
- Not the smallest cost, but the first negative result with its message.
- I think such an abstraction would not be much shorter: Result = InlineResult::FirstNegative(Result, expression)
Makes sense. I will try.
Wed, Jun 5
Interesting. I will try but next week.
Tue, Jun 4
Sun, Jun 2
Fri, May 31
added option switch-inst-prof-update-wrapper-strict to assert if prof data is inconsistent. It is off by default.
Probably it makes sense to enable prof branch_weights validation (patch D61179) in the same way.
Ok. This option will probably simplify catching such bugs.
Thu, May 30
Formatted and fixed one typo.
Yes. But in LLVM there are many places to fix. So we have to fix all of them at once and set the assertion here. The other option is to fix them one by one and tolerate such inconsistencies.
For example, SimplifyCFG is safe: it checks that the prof data is ok and only then makes its prof changes.
I have just submitted a fix. Please see D62656. Once it is landed this patch can be landed unchanged.
I decided to not include the crash test case as it would be caught by verification proposed in D61179.
Wed, May 29
Thank you very much for the revert, the analysis and the test.
I'm going make the wrapper class safe by introducing invalid sticky state.
Tue, May 28
May 27 2019
I considered this way and found it error prone. The wrapper stays alive after ConstantFoldTerminator() and developer may accidentally reuse it but the stored weights might be unsync with the underlying SwitchInst's weights changed by ConstantFoldTerminator().
- renamed SwitchInstProfBranchWeightsWrapper to SwitchInstProfUpdateWrapper
- explicitly narrowed scope of the SwitchInstProfUpdateWrapper object so it does not overlap with ConstantFoldTerminator()
- added a test case where switch turns into conditional branch
May 23 2019
May 22 2019
- renamed to SwitchInstProfUpdateWrapper
- added method descriptions
- added one assertion
May 21 2019
Done early return in eliminateDeadSwitchCases as suggested.
May 20 2019
See 62122 and its children.
See 62122 and its children.
- made use of SwitchInstProfBranchWeightsWrapper introduced in D62122.
- created new profmd tests instead of changing the existing tests
May 19 2019
May 15 2019
Apr 29 2019
changed verification level from Basic to Fast.
Apr 28 2019
Does it mean that you lgtm if I will remove the else part of the change (дштуы 604-606).