[llvm-commits] Div->[USF]Div Patch, Attempt #2

[Chris Lattner]

On Oct 25, 2006, at 12:15 PM, Reid Spencer wrote:

> Attached are two patch files to replace the DIV instruction with 3
> instructions: SDiv, UDiv, FDiv. The first file patches llvm. The  
> second
> file patches llvm-gcc4.
>
> This is the 2nd attempt to provide the patch.  All comments are  
> welcome.
>
> Reid.
> <DIV.patch>
> <DIV-llvmgcc.patch>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Overall, this patch looks very nice.  Please fix the one bug below  
(adding the regtest) and you are approved to check the whole mess in.



Final part (instcombine):

+Instruction *InstCombiner::visitFDiv(BinaryOperator &I) {
+  Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
+
+  return commonDivTransforms(I);
+}

You can delete Op0/Op1.


+          // FIXME: If the operand types don't match the type of the  
divide
+          // then don't attempt this transform. The code below  
doesn't have the
+          // logic to deal with a signed divide and an unsigned  
compare (and
+          // vice versa). This is because (x /s C1) <s C2  produces  
different
+          // results than (x /s C1) <u C2 or (x /u C1) <s C2 or even
+          // (x /u C1) <u C2.  Simply casting the operands and  
result won't
+          // work. :(  The if statement below tests that condition  
and bails
+          // if it finds it.
+          const Type* DivRHSTy = DivRHS->getType();
+          unsigned DivOpCode = LHSI->getOpcode();
+          if ((DivOpCode == Instruction::SDiv && DivRHSTy->isUnsigned 
()) ||
+              (DivOpCode == Instruction::UDiv && DivRHSTy->isSigned()))
+            break;


This is good but slightly over conservative.  Please allow the xform  
to happen if the comparison is an ==/!= comparison (i.e.  
SetCondInst::isEquality() returns true).



Instruction* InstCombiner::commonDivTransforms(BinaryOperator &I) {

Please add comments above each new function.  The visit* methods  
don't need comments above them.


     // (X / C1) / C2  -> X / (C1*C2)
     if (Instruction *LHS = dyn_cast<Instruction>(Op0))
       if (Instruction::BinaryOps(LHS->getOpcode()) == I.getOpcode())

This code is a bit simpler as:

     // (X / C1) / C2  -> X / (C1*C2)
     if (BinaryOperator *LHS = dyn_cast<BinaryOperator>(Op0))
       if (LHS->getOpcode() == I.getOpcode())


   // udiv X, (Select Cond, C1, C2) --> Select Cond, (shr X, C1),  
(shr X, C2)
   // where C1&C2 are powers of two.
...
                 X = InsertNewInstBefore(
                   new CastInst(X, X->getType()->getUnsignedVersion 
()), I);

This (and similar cases) is easier/cleaner with InsertCastBefore.


               // Finally, construct the select instruction and  
return it.
               return new SelectInst(SI->getOperand(0), TSI, FSI);

In the same xform, this needs to cast the result back to the right  
type if the operation was signed.  Please write a short regtest for  
this and check it in with your patch to verify that you get this  
right (add it to the end of div.ll or something).  Something like this:

int %test(int %X, int %Y, bool %C) {
	%A = select bool %C, int 1024, int 32
	%B = udiv int %Y, %A
	ret int %B
}

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061025/9803b95d/attachment.html>