[llvm-commits] Updated LowerSwitch patch

[Chris Lattner]

On Mar 3, 2007, at 4:11 PM, Anton Korobeynikov wrote:
> Hello, Everyone.
> Please find updated LowerSwitch patch (synced with latest APInt- 
> related
> changes).

Very interesting!  Can you give an example with before/after code?   
Please add this as a testcase as well.  Some code comments:


Meta-comment: this should *really* be merged into SelectionDAGISel,  
as nothing uses LowerSwitch any more.


+    struct Case {
+      Constant* Low;
+      Constant* High;
+      BasicBlock* BB;
+
+      Case(Constant* _Low = NULL, Constant* _High = NULL,
+           BasicBlock* _BB = NULL):
+        Low(_Low), High(_High), BB(_BB) { }
+    };
+
+    typedef std::vector<Case>           CaseVector;
+    typedef std::vector<Case>::iterator CaseItr;

Please rename Case to "CaseRange".



    struct CaseCmp {
      bool operator () (const LowerSwitch::Case& C1,
                        const LowerSwitch::Case& C2) {
-      const ConstantInt* CI1 = cast<const ConstantInt>(C1.first);
-      const ConstantInt* CI2 = cast<const ConstantInt>(C2.first);
-      return CI1->getValue().ult(CI2->getValue());
+      const ConstantInt* CI1 = cast<const ConstantInt>(C1.Low);
+      const ConstantInt* CI2 = cast<const ConstantInt>(C2.High);
+      return CI1->getValue().slt(CI2->getValue());
      }

I'm not sure that this is safe.  It seems that other parts of the  
code expect them to be sorted in ult, not slt, order.  For example,  
this code does:

   ICmpInst* Comp = new ICmpInst(ICmpInst::ICMP_ULT, Val, Pivot.Low,  
"Pivot");





+unsigned LowerSwitch::Clusterify(CaseVector& Cases, SwitchInst *SI)
+{

Please add a comment, indicating what this does.  The "{" goes on the  
same line as the proto.

+  std::list<Case> tmpCases;

std::list is really inefficient, please use a vector and drop the  
#include.  Once you do this, you can eliminate tmpCases, and just  
'clusterify' into 'Cases' directly.


+
+      if ((nextValue-currentValue==1) && (currentBB == nextBB)) {
+        I->High = J->High;
+        tmpCases.erase(J++);
+      } else {
+        I = J++;
+      }

Please add a comment indicating what this is doing.  Something like  
"If the two neighboring cases go to the same destination, merge them  
into a single case".

+  Cases.clear();

This shouldn't be needed.


For things like this:

+    if (I->Low != I->High)
+      // A range counts double, since it requires two compares.
+      ++numCmps;

Please format it like:

+    // A range counts double, since it requires two compares.
+    if (I->Low != I->High)
+      ++numCmps;

or put {}'s around the if block.


Please resend an updated version,

-Chris


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070303/cd7b780e/attachment.html>