PR1255: Reject commits that turns switch into confusing state

[Stepan Dyatkovskiy]

Hello Bob,

Please find revision info in .xls file I attached. If you need some help 
- let me know.

-Stepan.

Bob Wilson wrote:
> I don't see much value in sending patches for things to be reverted.  It would help to get your list of revisions, though -- just in case I'm missing some of them.
>
> The time consuming part of this is rewriting things that can't simply be reverted, e.g., I don't want to change the bit code format.
>
> On Sep 5, 2013, at 10:18 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
>
>> I think I can help you. Tomorrow I'll try to send you reject patches for all files that have to be rejected. I also will prepare you whole list of revisions that should be rejected.
>>
>> I just estimated it as 5-10 week, since it usually took 2-3 days before patch got LGTM, so 3*9 is 27 days.
>> But I can just send you 9 patches at once and it would be much faster, if you would like to of course..
>>
>> -Stepan.
>>
>> Bob Wilson wrote:
>>> OK.  Thanks for the background info.  I'm not sure what to suggest for an alternate approach to implementing case-range support.  For now, I'm going to go ahead and revert all the changes from the previous attempt.  It will take me a little while to do that, but it should be much less than 5-10 weeks.
>>>
>>> On Sep 4, 2013, at 7:51 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
>>>
>>>> Hello Bob,
>>>> Yes it is only the first patch.
>>>> I'd want to do it step-by-step.
>>>>
>>>> About bitcode. Yes I had changed format.
>>>> Reader has backward compatibility. It is important.
>>>> SwitchInst Writer has two branches for single-number cases and for case ranges. Currently we have no case-ranges. So we can just clean out case-ranges branch.
>>>>
>>>> About pushing things..
>>>> I don't think it would be good, since I touched only small part of llvm, but this approach supposes to affect almost every pass in LLVM. And it was already difficult (on this early stage) to keep things working.
>>>>
>>>> Actually the issue is:
>>>> 1. Every new patch in this approach makes llvm unstabler then ever. It affects new passes, while passes that already affected could still contain some bugs.
>>>> 2. People are too busy and can't give you consultations in time to keep code in good state. So sometimes you just think its ok, but instead not - people just can't track every man in community and missed you commit message. And when they find out that you did something wrong (Bitcode for example).. well it annoys them and makes you and everybody unhappy.
>>>> 3. Now I really don't think it was a good idea to enhance existing instruction. It is better to add new instruction, and remove old instruction then. What I wanted for ages is to cancel thess commits.
>>>>
>>>> Of course I still want to implement pr1255, but like a new instruction and I already have patches with working version (though some tests in test-suite are still failed).
>>>>
>>>> What I want to reject:
>>>>
>>>> Summary next files/classes would be affected:
>>>> lib/Transforms/Utils/Local.cpp
>>>> lib/Transforms/Utils/LowerSwitch.cpp
>>>> lib/Target/CPPBackend/CPPBackend.cpp
>>>> tools/llvm-diff/DifferenceEngine.cpp
>>>> lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
>>>> lib/IR/Verifier.cpp
>>>> lib/ExecutionEngine/Interpreter/Execution.cpp
>>>> BitcodeReader and Writer
>>>>
>>>> What to delete:
>>>>
>>>> Support/IntegersSubset.h
>>>> Support/IntegersSubsetMapping.h
>>>>
>>>> So summary this way of rejection assumes 9 patches (it would take about 5-10 weeks, I think). We could try to finish it till December. And perhaps add experimental instruction till May.
>>>>
>>>> Thanks.
>>>>
>>>> -Stepan.
>>>>
>>>> Bob Wilson wrote:
>>>>> The attached patch only touches one file: lib/Transforms/Utils/Local.cpp.  Did you forget to include the rest of it?
>>>>>
>>>>> I looked through some of your patches and was dismayed to see that you already changed the bit code format.  That's going to make it harder to simply revert all of your changes, since we need to maintain bit code compatibility.  Maybe it would make more sense to push forward and try to complete this project.  What remains to be done and what problems led you give up working on it?
>>>>>
>>>>> On Sep 2, 2013, at 5:25 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
>>>>>
>>>>>> Hi.
>>>>>> Being implementing PR1255: Case ranges, we stopped in the middle, in very confusing state. These patches reject SwitchInst to stable stage:
>>>>>> I propose to reject everything related to this issue till the r152532:
>>>>>> -- Remove IntegersSubset and friends,
>>>>>> -- Remove SwitchInst::getCaseValueEx (that returns IntegerSubset instance),
>>>>>> -- Remove IntegerSubsetMapping (reject SelectionDAGBuilder::Clusterify and LowerSwitch::Clusterify).
>>>>>>
>>>>>>
>>>>>>
>>>>>> Please find first patch in attachment for review.
>>>>>>
>>>>>> Patch rejects changes for local tranformations pass (lib/Tranform/Utils/Local.cpp).
>>>>>>
>>>>>> Revisions that would be rejected for this pass:
>>>>>> 159076, 157884, 157612, 157576, 157315.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> -Stepan.
>>>>>>
>>>>>> <Local.patch><Local-rejected-revs.txt>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: Summary.xls
Type: application/vnd.ms-excel
Size: 28160 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130906/dd38906a/attachment.xls>