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

This patch is *far* improved over the last one.  Nit picky stuff  
below.  Please make these changes, but there is no need to repost  
these diffs for review.


llvm part, without instcombine:

+// This function is used to obtain the correct opcode for an  
instruction when
+// an obsolete opcode is encountered. The OI parameter (OpcodeInfo)  
has both
+// an opcode and an "obsolete" flag. These are generated by the  
lexer and
+// the "obsolete" member will be true when the lexer encounters the  
token for
+// an obsolete opcode. For example, "div" was replaced by [usf]div  
but we need
+// to maintain backwards compatibility for asm files that still have  
the "div"
+// instruction. This function handles converting div -> [usf]div  
appropriately.
+static void
+sanitizeOpCode(OpcodeInfo<Instruction::BinaryOps> &OI, const  
PATypeHolder& PATy)
+{

Please convert this to a doxygen comment.  Please change the second  
argument to "const Type *Ty".



Bytecode/Reader.cpp:

+  // If this is a bytecode format that did not include the unreachable
+  // instruction, bump up the opcode number to adjust it.
+  if (hasNoUnreachableInst) {
+    if (Opcode >= Instruction::Unreachable &&
+        Opcode < 62) { // 62
+      ++Opcode;
+    }
+  }

What is the "// 62" comment?



+    case 11: // Rem
+      // As with "Div", make the signed/unsigned Rem instruction  
choice based
+      // on the type of the instruction.
+      if (ArgVec[0]->getType()->isFloatingPoint())
+        Opcode = Instruction::Rem;
+      else if (ArgVec[0]->getType()->isSigned())
+        Opcode = Instruction::Rem;
+      else
+        Opcode = Instruction::Rem;

Heh, so forward looking :), no need to change it.


+  // In version 5 and prior, the integer types were distinguished by  
sign.
+  // That is we have UIntTy and IntTy as well as ConstantSInt and
+  // ConstantUInt. In version 6, the integer types became signless  
so we
+  // need to note that we have signed integer types in prior versions.
+  bool hasSignedIntegers;

This is set but never checked, please remove it.


diff -t -d -u -p -5 -r1.94 ConstantFolding.cpp
--- lib/VMCore/ConstantFolding.cpp	20 Oct 2006 07:07:24 -0000	1.94
+++ lib/VMCore/ConstantFolding.cpp	25 Oct 2006 18:51:19 -0000
@@ -38,11 +38,13 @@ namespace {

...

+  static Constant *UDiv(const ConstantInt *V1, const ConstantInt *V2) {
+    if (V2->isNullValue())
+      return 0;
      if (V2->isAllOnesValue() &&              // MIN_INT / -1
          (BuiltinType)V1->getZExtValue() == -(BuiltinType)V1- 
 >getZExtValue())
        return 0;
+    BuiltinType R = (BuiltinType)(V1->getZExtValue() / V2- 
 >getZExtValue());
+    return ConstantInt::get(*Ty, R);
+  }


This check:
      if (V2->isAllOnesValue() &&              // MIN_INT / -1
          (BuiltinType)V1->getZExtValue() == -(BuiltinType)V1- 
 >getZExtValue())
        return 0;

Is not needed in the udiv case, it is over-conservative (yes, the  
original code was over-conservative in the same way).


-Chris

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