This patch implements one of the missing builtin functions specified in https://reviews.llvm.org/D111529
Details
Diff Detail
Event Timeline
Thanks for the patch! I left a suggestion inline.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
16734 | 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? |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
11337 | There's no need to add a new error message here. err_builtin_invalid_arg_type already uses %select to support different types in the message. You can just add a new option to the %select{} (done by |new option). You need to adjust the index passed to Diag() accordingly. | |
clang/lib/Sema/SemaChecking.cpp | ||
16734 | The latest version s till duplicates most of the logic unfortunately. I think it would be good to allow the caller of SemaBuiltinElementwiseMathOneArg to specify additional restrictions on the argument type. This could be done by adding an extra callback argument, like below: -bool Sema::SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall) { +bool Sema::SemaBuiltinElementwiseMathOneArg( + CallExpr *TheCall, + std::function<bool(QualType ArgTy, SourceLocation ArgLoc)> + ExtraCheckArgTy) { if (checkArgCount(*this, TheCall, 1)) return true; @@ -16707,16 +16734,10 @@ bool Sema::SemaBuiltinElementwiseMathOneArg(CallExpr *TheCall) { TheCall->setArg(0, A.get()); QualType TyA = A.get()->getType(); - if (checkMathBuiltinElementType(*this, ArgLoc, TyA)) + if (checkMathBuiltinElementType(*this, ArgLoc, TyA) || + ExtraCheckArgTy(TyA, ArgLoc)) return true; - QualType EltTy = TyA; - if (auto *VecTy = EltTy->getAs<VectorType>()) - EltTy = VecTy->getElementType(); - if (EltTy->isUnsignedIntegerType()) - return Diag(ArgLoc, diag::err_builtin_invalid_arg_type) - << 1 << /*signed integer or float ty*/ 3 << TyA; - TheCall->setType(TyA); return false; } At the call site, it would look something like - if (SemaBuiltinElementwiseMathOneArg(TheCall)) + if (SemaBuiltinElementwiseMathOneArg( + TheCall, [this](QualType ArgTy, SourceLocation ArgLoc) -> bool { + QualType EltTy = ArgTy; + if (auto *VecTy = EltTy->getAs<VectorType>()) + EltTy = VecTy->getElementType(); + if (EltTy->isUnsignedIntegerType()) + return Diag(ArgLoc, diag::err_builtin_invalid_arg_type) + << 1 << /*signed integer or float ty*/ 3 << ArgTy; + return false; + })) return ExprError(); break; |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
2104 | Could you add a comment at the top of the function, saying that this restricts the element type to signed integers or floating point types? | |
2108 | Could you add a comment here saying that this restricts the element type to floating point types only? | |
16708 | Could you swap the TyA and ArgLoc arguments to the function, so it is consistent with the earlier call to checkMathBuiltinElementType? (Something I missed when I posted the suggestion) |
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
649 | If we're doing ceil, should we be doing floor at the same time given how much overlap there should be in the implementations? | |
clang/lib/Sema/SemaChecking.cpp | ||
16707–16708 | 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? |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
16707–16708 |
Yeah that sounds better! |
I didn't know if I understand Aaron's words right, but I think I can update the patch first...
I think you understood my suggestion pretty well, thanks!
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
2108 | I don't think using a lambda here (or below) adds a whole lot of value given that it's only called once. I think it'd cleaner to hoist the lambda logic into the case block directly. WDYT? |
Thanks for the update, it looks like it should be in line with @aaron.ballman's suggestions!
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).
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
2102 | nit: typo estricts -> restricts? | |
2119 | nit: typo estricts -> restricts? It should also say _ceil instead of _abs, right? | |
16728 | w |
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@junz.org
Cheers
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
16707–16708 |
Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean. :( Sorry about that, I'm completely a newbie...
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! | |
16734 |
Hi, Florian, Thanks for your suggestion! I have updated the patch, PTAL :-) |
Thank you for the new functionality, I've committed on your behalf in 8680f951c21e675a110e79c9b8dc59bb94290b01
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
16707–16708 |
No apologies necessary, we've all started there! :-)
The way you implemented it and the way I suggested it are functionally the same outcome. PrepareBuiltinElementwiseMathOneArgCall() eventually calls TheCall->setType(TyA); to set the return type of the call expression to be the same as the argument type. After calling that method, you can either do QualType ArgTy = TheCall->getArg(0)->getType(); as you were doing or do QualType ArgTy = TheCall->getType(); to get that adjusted type. The way you've written it currently is correct and likely a more readable solution. My suggestion would have ever-so-slightly better performance because it doesn't call getArg(0), but I think we should prefer your approach because it's more readable. That's why I accepted the patch as-is. Does that clarify? |
If we're doing ceil, should we be doing floor at the same time given how much overlap there should be in the implementations?