[llvm-commits] [llvm] r55753 - in /llvm/trunk: include/llvm/ include/llvm/CodeGen/ lib/CodeGen/ lib/CodeGen/SelectionDAG/ lib/Target/ARM/ lib/Target/Alpha/ lib/Target/CellSPU/ lib/Target/IA64/ lib/Target/PowerPC/ lib/Target/Sparc/ lib/Target/X86/

[Chris Lattner]

On Sep 3, 2008, at 5:47 PM, Dale Johannesen wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=55753&view=rev
> Log:
> Add intrinsics for log, log2, log10, exp, exp2.
> No functional change (and no FE change to generate them).

Hi Dale,

Thanks for working on this.  Please factor this code a little bit:

> +++ llvm/trunk/lib/CodeGen/IntrinsicLowering.cpp Wed Sep  3 19:47:13  
> 2008
> @@ -160,6 +160,81 @@
>                                I->arg_begin()->getType());
>         }
>         break;
> +      case Intrinsic::log:
> +        switch((int)I->arg_begin()->getType()->getTypeID()) {
> +        case Type::FloatTyID:
> +          EnsureFunctionExists(M, "logf", I->arg_begin(), I- 
> >arg_end(),
> +                               Type::FloatTy);
> +        case Type::DoubleTyID:
> +          EnsureFunctionExists(M, "log", I->arg_begin(), I- 
> >arg_end(),
> +                               Type::DoubleTy);
> +        case Type::X86_FP80TyID:
> +        case Type::FP128TyID:
> +        case Type::PPC_FP128TyID:
> +          EnsureFunctionExists(M, "logl", I->arg_begin(), I- 
> >arg_end(),
> +                               I->arg_begin()->getType());
> +        }
> +        break;
> +      case Intrinsic::log2:
> +        switch((int)I->arg_begin()->getType()->getTypeID()) {
> +        case Type::FloatTyID:
> +          EnsureFunctionExists(M, "log2f", I->arg_begin(), I- 
> >arg_end(),
> +                               Type::FloatTy);
> +        case Type::DoubleTyID:
> +          EnsureFunctionExists(M, "log2", I->arg_begin(), I- 
> >arg_end(),
> +                               Type::DoubleTy);
> +        case Type::X86_FP80TyID:
> +        case Type::FP128TyID:
> +        case Type::PPC_FP128TyID:
> +          EnsureFunctionExists(M, "log2l", I->arg_begin(), I- 
> >arg_end(),
> +                               I->arg_begin()->getType());
> +        }
> +        break;

this pattern is repeated several times.  Please introduce a new  
function, and something like:

case Intrinsic::log:
   EnsureFPFunctionExists(I->arg_begin()->getType(), "logf", "log",  
"logl");
   break;
case Intrinsic::log2:
   EnsureFPFunctionExists(I->arg_begin()->getType(), "log2f", "log2",  
"log2l");
   break;
etc

> @@ -857,6 +932,144 @@
> +  case Intrinsic::log: {
> +    static Constant *logfFCache = 0;
> +    static Constant *logFCache = 0;
> +    static Constant *logLDCache = 0;
> +    switch (CI->getOperand(1)->getType()->getTypeID()) {
> +    default: assert(0 && "Invalid type in log"); abort();
> +    case Type::FloatTyID:
> +      ReplaceCallWith("logf", CI, CI->op_begin()+1, CI->op_end(),
> +                    Type::FloatTy, logfFCache);
> +      break;
> +    case Type::DoubleTyID:
> +      ReplaceCallWith("log", CI, CI->op_begin()+1, CI->op_end(),
> +                    Type::DoubleTy, logFCache);
> +      break;
> +    case Type::X86_FP80TyID:
> +    case Type::FP128TyID:
> +    case Type::PPC_FP128TyID:
> +      ReplaceCallWith("logl", CI, CI->op_begin()+1, CI->op_end(),
> +                    CI->getOperand(1)->getType(), logLDCache);
> +      break;
> +    }
> +    break;
> +  }

Likewise here, this codegen code can be factored to call a helper.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp Wed Sep  3  
> 19:47:13 2008
> @@ -243,6 +243,16 @@
>   setOperationAction(ISD::FCOS     , MVT::f64, Expand);
>   setOperationAction(ISD::FREM     , MVT::f64, Expand);
>   setOperationAction(ISD::FREM     , MVT::f32, Expand);
> +  setOperationAction(ISD::FLOG     , MVT::f64, Expand);
> +  setOperationAction(ISD::FLOG     , MVT::f32, Expand);
> +  setOperationAction(ISD::FLOG2    , MVT::f64, Expand);
> +  setOperationAction(ISD::FLOG2    , MVT::f32, Expand);
> +  setOperationAction(ISD::FLOG10   , MVT::f64, Expand);
> +  setOperationAction(ISD::FLOG10   , MVT::f32, Expand);
> +  setOperationAction(ISD::FEXP     , MVT::f64, Expand);
> +  setOperationAction(ISD::FEXP     , MVT::f32, Expand);
> +  setOperationAction(ISD::FEXP2    , MVT::f64, Expand);
> +  setOperationAction(ISD::FEXP2    , MVT::f32, Expand);

Instead of doing this in every target, why not make the default be  
Expand, and put this once in TargetLowering::TargetLowering, like we  
do for ISD::TRAP and ConstantFP etc.  This lets the target get a sane  
default behavior and override it in the derived constructor if they  
really want to do something funny.

>

-Chris