[PATCH] D114688: [Clang] Add __builtin_elementwise_ceil

[Jun Zhang via Phabricator via cfe-commits]

junaire added a comment.

In D114688#3176567 <https://reviews.llvm.org/D114688#3176567>, @aaron.ballman wrote:

> In D114688#3176433 <https://reviews.llvm.org/D114688#3176433>, @fhahn wrote:
>
>> I think it might be good to split off the refactoring of `SemaBuiltinElementwiseMathOneArg`-> `PrepareBuiltinElementwiseMathOneArgCall` and the update for `BI__builtin_elementwise_abs` into a separate, non functional change (NFC).
>
> FWIW, I would not be opposed to that, but I don't insist either. LGTM!

Hi, @fhahn @aaron.ballman . Thanks for your patient reviews and valuable suggestions! I think I might not apply the extra suggestion as I don't really understand that, sorry... BTW, if you think this patch is ready, could you please commit this for me?

You can use:
Jun Zhang jun at junz.org

Cheers



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16727-16728
   QualType TyA = A.get()->getType();
-  if (checkMathBuiltinElementType(*this, ArgLoc, TyA))
+  if (checkMathBuiltinElementType(*this, ArgLoc, TyA) ||
+      ExtraCheck(TyA, ArgLoc))
     return true;
----------------
fhahn wrote:
> aaron.ballman wrote:
> > In some ways, it's a not really ideal that we split the type checking like this. `SemaBuiltinElementwiseMathOneArg()` checks some type validity and the caller checks some more type validity, so there's no one location to know what types are actually expected for any given builtin calling this.
> > 
> > Would it be reasonable to instead hoist all the type checking code out of `SemaBuiltinElementwiseMathOneArg()` and not pass in a functor to it, but instead check the `CallExpr` type after the call returns? We could also fix up the function name to be a bit less nonsensical at the same time, like renaming it to `PrepareBuiltinElementwiseMathOneArgCall()` or something along those lines?
> > Would it be reasonable to instead hoist all the type checking code out of SemaBuiltinElementwiseMathOneArg() and not pass in a functor to it, but instead check the CallExpr type after the call returns?
> 
> Yeah that sounds better!
> In some ways, it's not really ideal that we split the type checking like this. `SemaBuiltinElementwiseMathOneArg()` checks some type validity and the caller checks some more type validity, so there's no one location to know what types are actually expected for any given builtin calling this.
> 
> Would it be reasonable to instead hoist all the type checking code out of `SemaBuiltinElementwiseMathOneArg()` and not pass in a functor to it, but instead check the `CallExpr` type after the call returns? We could also fix up the function name to be a bit less nonsensical at the same time, like renaming it to `PrepareBuiltinElementwiseMathOneArgCall()` or something along those lines?

Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean. :( Sorry about that, I'm completely a newbie... 

>check the `CallExpr` type after the call returns?

What does this mean? From my understanding, we only need to check the argument type, right? Or do you mean something like:
```
  TheCall->setType(TyA);  // inside checking function
...

TheCall->getType(); // call side
```
Do I understand you correctly? Please don't hesitate to point out any mistake I made, thanks!



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16721
 
+bool Sema::SemaBuiltinElementwiseMathFloatArg(CallExpr *TheCall) {
+  if (checkArgCount(*this, TheCall, 1))
----------------
fhahn wrote:
> If I understand correctly, this is similar to `SemaBuiltinElementwiseMathOneArg`, but with the additional restriction to only allow floating point element types? 
> 
> Do you think it would be possible to extend `SemaBuiltinElementwiseMathOneArg` to handle this case directly, without needing to duplicate most of the other logic?
> If I understand correctly, this is similar to `SemaBuiltinElementwiseMathOneArg`, but with the additional restriction to only allow floating point element types? 
> 
> Do you think it would be possible to extend `SemaBuiltinElementwiseMathOneArg` to handle this case directly, without needing to duplicate most of the other logic?

Hi, Florian, Thanks for your suggestion! I have updated the patch, PTAL :-)


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

https://reviews.llvm.org/D114688