PR1255: Reject commits that turns switch into confusing state

Stepan Dyatkovskiy stpworld at narod.ru
Wed Sep 4 07:51:03 PDT 2013


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




More information about the llvm-commits mailing list