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

[Reid Spencer]

All Done.

On Wed, 2006-10-25 at 21:03 -0700, Chris Lattner wrote:
> 
> 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
> 
>