Switch case-ranges, new approach (PR1255).

[Stepan Dyatkovskiy]

Hi Evan,

Thank you for reply.

 > 1. Rather than switchr, would it be clearer to spell it out? Say 
switchrange?
'switchrange' is OK to me. Indeed: I wanted to hear somebody's opinion 
about its name. Though it will be discussed, the final name, I suppose.

 > If you leaving TODO in the code (which you should avoid if possible), 
please explain what's left to do.
Definitely. Some todo's I've kept in passes without comments, but with 
implicit message (to myself) "implement support for new instruction 
here". Will fix.

 > 5.
 > +template <class CaseItTy,
 > +          class SwitchRInstTy,
 > +          class BasicBlockTy>
 > +class SwitchRInstCaseItImplBase;
Hm.. There are same implementation for CaseIt and ConstCaseIt.Here (and, 
in general, for all const/non-const iterators) I don't know what is 
better: duplicate code or create ugly-templated but unified implementation.

Thanks! Will ping in next two weeks then.

P.S.: The actual state of my PR1255 implementation is available here:
git://github.com/kaomoneus/llvm.case-ranges.git, branch 'switchr'.
Or by command:
git clone -b switchr git://github.com/kaomoneus/llvm.case-ranges.git

-Stepan.

Evan Cheng wrote:
> Hi,
>
> Unfortunately there is quite a lot bit of code to go through. We do need Chris to sign-off on the IR extension and I believe he is busy for the next couple of weeks. You might want to ping us again in about 2 weeks.
>
> I do have a few early comments:
>
> 1. Rather than switchr, would it be clearer to spell it out? Say switchrange?
> 2. Can you do TEST=beta-compare and compare the number of instructions being generated?
> 3.
> +  // TODO:
> +  if (SwitchRInst *SI = dyn_cast<SwitchRInst>(&TI)) {
> +    Succs.assign(TI.getNumSuccessors(), true);
> +    return;
> +  }
> +
>
> If you leaving TODO in the code (which you should avoid if possible), please explain what's left to do.
> 4.
> +//===---------------------------------------------------------------------------
> +/// SwitchRInst - Multiway switch
> +///
>
> This doesn't appear to be an precise description.
> 5.
> +template <class CaseItTy,
> +          class SwitchRInstTy,
> +          class BasicBlockTy>
> +class SwitchRInstCaseItImplBase;
>
> Why is it necessary to use template?
>
> Thanks,
>
> Evan
>
> On May 24, 2013, at 1:30 PM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
>
>> Hi all!
>> Glad to report you: clang with forced new 'switchr' instruction have passed all the test-suite tests on x86_64.
>> Here are the nightly reports and archive with patches in attachment. Its not a performance test, so time measurements are not pretty well here.
>>
>> Would you please review this approach?
>>
>> Thanks!
>>
>> -Stepan.
>>
>> Stepan Dyatkovskiy wrote:
>>> Hello, I propose new approach to switch case-ranges
>>> implementation (PR1255).
>>>
>>> Don't modify SwitchInst, but implement new instruction.
>>>
>>> In this case:
>>> 1. We don't affect current functionality and tests.
>>> 2. We keep backward compatibility in natural way. If you need current
>>> functionality (without case ranges) just use 'switch'.
>>>
>>> Here I have attached set of patches:
>>>
>>> -- New instruction itself. It is called 'SwitchRInst'. Implementation
>>>     is ConstantRange based. Internally switch is represented as just an
>>>     array of integers. Though i-th and i+1 integer represents a range,
>>>     and i-th integer is linked with this-range successor. This is a
>>>     result of implementation discussions in IRC and ML.
>>>     Patch name: switchr-2013-05-07-instruction.patch
>>>
>>> -- Parser/AsmWriter patch. After this patch IR parser will
>>>     recognize new instruction and its syntax ('switchr' keyword will
>>>     be added).
>>>     Patch contains unit-tests.
>>>     Patch name: switchr-2013-05-07-parser-writer.patch
>>>     Requires: switchr-2013-05-07-instruction.patch
>>>
>>> -- Bitcode patch. Patch adds new instruction support
>>>     for .bc files with.
>>>     Patch contains unit-tests.
>>>     Patch name: switchr-2013-05-07-bitcode.patch
>>>     Requires: switchr-2013-05-07-instruction.patch
>>>              [switchr-2013-05-07-parser-writer.patch] (for unit-tests)
>>>
>>> -- "Essential passes" patch. This patch finally enables 'switchr':
>>>     it becomes absultely legal on LLVM level. Patch updates next passes:
>>>     * SelectionDAGBuilder: lowers 'switchr';
>>>     * IRBuilder: "Create" method;
>>>     * SCCP: just a stub, that marks all 'switchr' successors
>>>     as reachable.
>>>     Patch contains unit-tests.
>>>     Patch name: switchr-2013-05-07-essential-passes.patch
>>>     Requires: switchr-2013-05-07-instruction.patch
>>>              [switchr-2013-05-07-parser-writer.patch] (for unit-tests)
>>>
>>> -- Switch-to-SwitchR pass patch. Adds function pass that replaces
>>>     'switch' instructions with 'switchr'.
>>>     Patch name: switchr-2013-05-07-switch-to-switchr.patch
>>>
>>> -- Clang patch. Adds '-fforce-switchr' option: with this flag clang
>>>     will use 'switchr' instructions instead of 'switch'. Actually, it
>>>     inserts SwitchToSwitchR patch.
>>>     Patch contains unit-tests.
>>>     Patch name: switchr-clang-force-flag-2013-05-07.patch
>>>     Requires: switchr-2013-05-07-instruction.patch
>>>               switchr-2013-05-07-switch-to-switchr.patch
>>>              [switchr-2013-05-07-parser-writer.patch] (for unit-tests)
>>>
>>> Also I've attached summary LLVM patch (all-in-one).
>>>
>>> -Stepan.
>>
>> <switchr-2013-05-24.tgz><r181782-report.nightly.txt><switchr-181782-report.nightly.txt>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>