Switch case-ranges, new approach (PR1255).

[Evan Cheng]

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