Page MenuHomePhabricator

[Clang] Add __builtin_elementwise_ceil
ClosedPublic

Authored by junaire on Nov 28 2021, 10:25 PM.

Details

Summary

This patch implements one of the missing builtin functions specified in https://reviews.llvm.org/D111529

Diff Detail

Event Timeline

junaire requested review of this revision.Nov 28 2021, 10:25 PM
junaire created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2021, 10:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the patch! I left a suggestion inline.

clang/lib/Sema/SemaChecking.cpp
16746

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?

junaire updated this revision to Diff 390912.Nov 30 2021, 10:35 PM

Reuse existing code.

junaire updated this revision to Diff 390914.Nov 30 2021, 10:47 PM

Adjust code style

fhahn added inline comments.Dec 1 2021, 3:57 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11338

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
16746

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;
junaire updated this revision to Diff 391016.Dec 1 2021, 7:23 AM
junaire marked an inline comment as done.

Apply some awesome suggestions.

fhahn added inline comments.Dec 1 2021, 7:39 AM
clang/lib/Sema/SemaChecking.cpp
2103

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?

2143

Could you add a comment here saying that this restricts the element type to floating point types only?

16738

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)

aaron.ballman added inline comments.Dec 1 2021, 8:17 AM
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
16737–16738

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?

fhahn added inline comments.Dec 1 2021, 8:19 AM
clang/lib/Sema/SemaChecking.cpp
16737–16738

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!

junaire updated this revision to Diff 391316.Dec 2 2021, 6:56 AM

I didn't know if I understand Aaron's words right, but I think I can update the patch first...

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
2107

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?

junaire updated this revision to Diff 391574.Dec 3 2021, 1:07 AM

No more lambdas

fhahn added a comment.Dec 7 2021, 7:47 AM

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
2101–2104

nit: typo estricts -> restricts?

2121

nit: typo estricts -> restricts? It should also say _ceil instead of _abs, right?

16739–16740

w

junaire updated this revision to Diff 392405.Dec 7 2021, 8:07 AM

Fix typos

aaron.ballman accepted this revision.Dec 7 2021, 8:21 AM

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!

This revision is now accepted and ready to land.Dec 7 2021, 8:21 AM

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@junz.org

Cheers

clang/lib/Sema/SemaChecking.cpp
16737–16738

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!

16746

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 :-)

aaron.ballman closed this revision.Dec 8 2021, 5:31 AM

Thank you for the new functionality, I've committed on your behalf in 8680f951c21e675a110e79c9b8dc59bb94290b01

clang/lib/Sema/SemaChecking.cpp
16737–16738

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

No apologies necessary, we've all started there! :-)

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!

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?