[PATCH] D116161: [Clang] Add an overload for emitUnaryBuiltin.

[Jun Zhang via Phabricator via cfe-commits]

junaire added a comment.

In D116161#3209292 <https://reviews.llvm.org/D116161#3209292>, @fhahn wrote:

> In D116161#3209286 <https://reviews.llvm.org/D116161#3209286>, @junaire wrote:
>
>>   35:  %0 = load float, float* %f1.addr, align 4 
>>   36:  %1 = load float, float* %f1.addr, align 4 
>>   37:  %elt.abs = call float @llvm.fabs.f32(float %1) 
>
> It looks like the argument expression is evaluated twice. Did you remove the `    Value *Op0 = EmitScalarExpr(E->getArg(0));` calls?

Well, for example:
For `__builtin_elementwise_abs`, we have code like:

  case Builtin::BI__builtin_elementwise_abs: {
    Value *Op0 = EmitScalarExpr(E->getArg(0));
    Value *Result;
    if (Op0->getType()->isIntOrIntVectorTy())
      Result = Builder.CreateBinaryIntrinsic(
          llvm::Intrinsic::abs, Op0, Builder.getFalse(), nullptr, "elt.abs");
    else
      Result = Builder.CreateUnaryIntrinsic(llvm::Intrinsic::fabs, Op0, nullptr,
                                            "elt.abs");
  
    return RValue::get(Result);
  }

If we use `emitUnaryBuiltin`, we are supposed to do something like:

  Result = emitUnaryBuiltin(*this, E, llvm::Intrinsic::fabs, "elt.abs");

We need to pass `CallExpr* E` to the function to meet its interface, and it calls `CGF.EmitScalarExpr(E->getArg(0));` inside. Maybe this is what you said about the argument expression being evaluated twice? I think it is avoidable if we don't change the function's interface.

Maybe we can have something like:

  static Value *emitUnaryBuiltin(CodeGenFunction &CGF, Value* Op0,
                                 unsigned IntrinsicID, llvm::StringRef Name) {
    return CGF.Builder.CreateUnaryIntrinsic(IntrinsicID, Op0, nullptr, Name);
  }

Then for `__builtin_elementwise_abs` we can have:

  Result = emitUnaryBuiltin(*this, Op0, llvm::Intrinsic::fabs, "elt.abs");

and for `__builtin_elementwise_ceil` have:

  return RValue::get(
      emitUnaryBuiltin(*this, EmitScalarExpr(E->getArg(0)), llvm::Intrinsic::ceil, "elt.ceil"));

WDYT? Well, franking speaking I think this one-line function is ugly but I can't come up with a more elegant solution, I would appreciate it if you can offer some suggestions. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116161/new/

https://reviews.llvm.org/D116161