[llvm-commits] Review request: ARM vector ctlz should use target-independent intrinsic

[Benjamin Kramer]

The clang part looks good, but it should have a test case. Comments for the LLVM part below.

On 11.07.2012, at 16:38, Joel Jones <joel_k_jones at apple.com> wrote:

> Index: lib/VMCore/AutoUpgrade.cpp
> ===================================================================
> --- lib/VMCore/AutoUpgrade.cpp	(revision 160031)
> +++ lib/VMCore/AutoUpgrade.cpp	(working copy)
> @@ -52,6 +52,22 @@
>  
>    switch (Name[0]) {
>    default: break;
> +  case 'a': {
> +    if (Name.startswith("arm.neon.vclz")) {
> +      // llvm.arm.neon.vclz.* -> llvm.ctlz.*
> +      Twine newName = "llvm.ctlz." + Name.substr(14);

Please don't use local Twine variables, they rely on subtle lifetime rules and should only be used as a direct parameter in function calls.

> +      Type* args[2] = {
> +        F->arg_begin()->getType(), 
> +        Type::getInt1Ty(F->getContext())
> +      };
> +      // Can't use Intrinsic::getDeclaration here as it adds a ".i1" to
> +      // the end of the name.
> +      FunctionType* fType = FunctionType::get(F->getReturnType(), args, false);
> +      NewFn = Function::Create(fType, F->getLinkage(), newName, F->getParent());
> +      return true;
> +    }
> +    break;
> +  }
>    case 'c': {
>      if (Name.startswith("ctlz.") && F->arg_size() == 1) {
>        F->setName(Name + ".old");
> @@ -295,6 +311,15 @@
>      CI->eraseFromParent();
>      return;
>  
> +  case Intrinsic::arm_neon_vclz: {
> +    // llvm.arm.neon.vclz.* -> llvm.ctlz.*
> +    Twine newName = "llvm.ctlz." + Name.substr(14);

dito

> +    CI->replaceAllUsesWith(Builder.CreateCall2(NewFn, CI->getArgOperand(0),
> +                                               Builder.getFalse(), newName));
> +    CI->eraseFromParent();
> +    return;
> +  }
> +
>    case Intrinsic::x86_xop_vfrcz_ss:
>    case Intrinsic::x86_xop_vfrcz_sd:
>      CI->replaceAllUsesWith(Builder.CreateCall(NewFn, CI->getArgOperand(1),