- User Since
- Jul 4 2017, 1:39 AM (118 w, 5 d)
Sep 11 2019
Aug 12 2019
Aug 7 2019
Jul 9 2019
done. D64233 is landed.
Closed by commit rL365439: Prepare for making SwitchInstProfUpdateWrapper strict (authored by yrouban).
yrouban committed rG592f44a7e75b: Prepare for making SwitchInstProfUpdateWrapper strict (authored by yrouban).
Jul 8 2019
Jul 4 2019
extracted the preparation patch to the separate patch D64233.
Jul 3 2019
Changed assert(false) to llvm_unreachable() for release builds to fail on inconsistencies. Though it seems to be too strict as the code can tolerate such inconsistencies.
Jul 2 2019
Frankly speaking I'm not 100% sure that the code is safe. There could be projects that might hit the assertion. E.g I have to fix one pass internally at Azul. I tested LLVM monorepo make check-all and our Azul Zing test suits. So, I believe that the only way to find issues is to land this check D64061 and the verifyer D61179 one by one with some reasonable interval (one week?).
Jul 1 2019
Jun 26 2019
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
Jun 24 2019
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.
Jun 21 2019
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.
Jun 20 2019
Jun 18 2019
Rebased to the latest HEAD. Please review. Its sibling patches have been landed.
Jun 17 2019
Jun 16 2019
Jun 14 2019
- 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.
Jun 11 2019
Jun 9 2019
- 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.
Jun 5 2019
Interesting. I will try but next week.
Jun 4 2019
Jun 2 2019
May 31 2019
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.
May 30 2019
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.
May 29 2019
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.
May 28 2019
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