Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Ensure fixed point conversions work in C++ #68344

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

PiJoules
Copy link
Contributor

@PiJoules PiJoules commented Oct 5, 2023

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 1c93781918135b16b19f7a9758040e0e18420617 59582cb54562c3c6933003267a46aebb50c67b42 -- clang/include/clang/Sema/Overload.h clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaExprCXX.cpp clang/lib/Sema/SemaOverload.cpp clang/test/Frontend/fixed_point_conversions.c
View the diff from clang-format here.
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 081b568762..c3645f5a41 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4511,29 +4511,33 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
            "Attempting implicit fixed point conversion without a fixed "
            "point operand");
     if (FromType->isFloatingType())
-      From = ImpCastExprToType(From, ToType, CK_FloatingToFixedPoint,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From =
+          ImpCastExprToType(From, ToType, CK_FloatingToFixedPoint, VK_PRValue,
+                            /*BasePath=*/nullptr, CCK)
+              .get();
     else if (ToType->isFloatingType())
-      From = ImpCastExprToType(From, ToType, CK_FixedPointToFloating,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From =
+          ImpCastExprToType(From, ToType, CK_FixedPointToFloating, VK_PRValue,
+                            /*BasePath=*/nullptr, CCK)
+              .get();
     else if (FromType->isIntegralType(Context))
-      From = ImpCastExprToType(From, ToType, CK_IntegralToFixedPoint,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From =
+          ImpCastExprToType(From, ToType, CK_IntegralToFixedPoint, VK_PRValue,
+                            /*BasePath=*/nullptr, CCK)
+              .get();
     else if (ToType->isIntegralType(Context))
-      From = ImpCastExprToType(From, ToType, CK_FixedPointToIntegral,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From =
+          ImpCastExprToType(From, ToType, CK_FixedPointToIntegral, VK_PRValue,
+                            /*BasePath=*/nullptr, CCK)
+              .get();
     else if (ToType->isBooleanType())
-      From = ImpCastExprToType(From, ToType, CK_FixedPointToBoolean,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From = ImpCastExprToType(From, ToType, CK_FixedPointToBoolean, VK_PRValue,
+                               /*BasePath=*/nullptr, CCK)
+                 .get();
     else
-      From = ImpCastExprToType(From, ToType, CK_FixedPointCast,
-                               VK_PRValue,
-                               /*BasePath=*/nullptr, CCK).get();
+      From = ImpCastExprToType(From, ToType, CK_FixedPointCast, VK_PRValue,
+                               /*BasePath=*/nullptr, CCK)
+                 .get();
     break;
 
   case ICK_Compatible_Conversion:
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 9800d7f1c9..16ae9f8036 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -123,43 +123,42 @@ CompareDerivedToBaseConversions(Sema &S, SourceLocation Loc,
 /// GetConversionRank - Retrieve the implicit conversion rank
 /// corresponding to the given implicit conversion kind.
 ImplicitConversionRank clang::GetConversionRank(ImplicitConversionKind Kind) {
-  static const ImplicitConversionRank
-    Rank[] = {
-    ICR_Exact_Match,
-    ICR_Exact_Match,
-    ICR_Exact_Match,
-    ICR_Exact_Match,
-    ICR_Exact_Match,
-    ICR_Exact_Match,
-    ICR_Promotion,
-    ICR_Promotion,
-    ICR_Promotion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_OCL_Scalar_Widening,
-    ICR_Complex_Real_Conversion,
-    ICR_Conversion,
-    ICR_Conversion,
-    ICR_Writeback_Conversion,
-    ICR_Exact_Match, // NOTE(gbiv): This may not be completely right --
-                     // it was omitted by the patch that added
-                     // ICK_Zero_Event_Conversion
-    ICR_Exact_Match, // NOTE(ctopper): This may not be completely right --
-                     // it was omitted by the patch that added
-                     // ICK_Zero_Queue_Conversion
-    ICR_C_Conversion,
-    ICR_C_Conversion_Extension,
-    ICR_Conversion,
+  static const ImplicitConversionRank Rank[] = {
+      ICR_Exact_Match,
+      ICR_Exact_Match,
+      ICR_Exact_Match,
+      ICR_Exact_Match,
+      ICR_Exact_Match,
+      ICR_Exact_Match,
+      ICR_Promotion,
+      ICR_Promotion,
+      ICR_Promotion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_OCL_Scalar_Widening,
+      ICR_Complex_Real_Conversion,
+      ICR_Conversion,
+      ICR_Conversion,
+      ICR_Writeback_Conversion,
+      ICR_Exact_Match, // NOTE(gbiv): This may not be completely right --
+                       // it was omitted by the patch that added
+                       // ICK_Zero_Event_Conversion
+      ICR_Exact_Match, // NOTE(ctopper): This may not be completely right --
+                       // it was omitted by the patch that added
+                       // ICK_Zero_Queue_Conversion
+      ICR_C_Conversion,
+      ICR_C_Conversion_Extension,
+      ICR_Conversion,
   };
   static_assert(std::size(Rank) == (int)ICK_Num_Conversion_Kinds);
   return Rank[(int)Kind];
@@ -168,38 +167,38 @@ ImplicitConversionRank clang::GetConversionRank(ImplicitConversionKind Kind) {
 /// GetImplicitConversionName - Return the name of this kind of
 /// implicit conversion.
 static const char* GetImplicitConversionName(ImplicitConversionKind Kind) {
-  static const char* const Name[] = {
-    "No conversion",
-    "Lvalue-to-rvalue",
-    "Array-to-pointer",
-    "Function-to-pointer",
-    "Function pointer conversion",
-    "Qualification",
-    "Integral promotion",
-    "Floating point promotion",
-    "Complex promotion",
-    "Integral conversion",
-    "Floating conversion",
-    "Complex conversion",
-    "Floating-integral conversion",
-    "Pointer conversion",
-    "Pointer-to-member conversion",
-    "Boolean conversion",
-    "Compatible-types conversion",
-    "Derived-to-base conversion",
-    "Vector conversion",
-    "SVE Vector conversion",
-    "RVV Vector conversion",
-    "Vector splat",
-    "Complex-real conversion",
-    "Block Pointer conversion",
-    "Transparent Union Conversion",
-    "Writeback conversion",
-    "OpenCL Zero Event Conversion",
-    "OpenCL Zero Queue Conversion",
-    "C specific type conversion",
-    "Incompatible pointer conversion",
-    "Fixed point conversion",
+  static const char *const Name[] = {
+      "No conversion",
+      "Lvalue-to-rvalue",
+      "Array-to-pointer",
+      "Function-to-pointer",
+      "Function pointer conversion",
+      "Qualification",
+      "Integral promotion",
+      "Floating point promotion",
+      "Complex promotion",
+      "Integral conversion",
+      "Floating conversion",
+      "Complex conversion",
+      "Floating-integral conversion",
+      "Pointer conversion",
+      "Pointer-to-member conversion",
+      "Boolean conversion",
+      "Compatible-types conversion",
+      "Derived-to-base conversion",
+      "Vector conversion",
+      "SVE Vector conversion",
+      "RVV Vector conversion",
+      "Vector splat",
+      "Complex-real conversion",
+      "Block Pointer conversion",
+      "Transparent Union Conversion",
+      "Writeback conversion",
+      "OpenCL Zero Event Conversion",
+      "OpenCL Zero Queue Conversion",
+      "C specific type conversion",
+      "Incompatible pointer conversion",
+      "Fixed point conversion",
   };
   static_assert(std::size(Name) == (int)ICK_Num_Conversion_Kinds);
   return Name[Kind];

@PiJoules PiJoules force-pushed the extend-fixed-point-semantics-to-c++ branch from 631458f to 1eb1638 Compare October 5, 2023 21:26
@PiJoules PiJoules requested a review from rjmccall October 5, 2023 21:27
@@ -2192,6 +2194,9 @@ static bool IsStandardConversion(Sema &S, Expr* From, QualType ToType,
From->isIntegerConstantExpr(S.getASTContext())) {
SCS.Second = ICK_Compatible_Conversion;
FromType = ToType;
} else if (ToType->isFixedPointType() || FromType->isFixedPointType()) {
SCS.Second = ICK_Fixed_Point_Conversion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's not really any value in breaking this down here? Seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah relating to the block in SemaExprCXX seemed easiest to me.

@@ -156,7 +156,8 @@ ImplicitConversionRank clang::GetConversionRank(ImplicitConversionKind Kind) {
// it was omitted by the patch that added
// ICK_Zero_Queue_Conversion
ICR_C_Conversion,
ICR_C_Conversion_Extension
ICR_C_Conversion_Extension,
ICR_C_Conversion_Extension,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the consequence of using ICR_C_Conversion_Extension here instead of just an ordinary conversion? In a sense, this isn't an extension in the same way that some of the other conversions are — these are just fundamental types that C++ hasn't incorporated into the standard yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ICR_C_Conversion_Extension I picked pretty much because of the Extension bit. Functionally, using ICR_Conversion doesn't change anything since none of the other implicit conversion kinds that have a rank from ICR_Conversion to ICR_C_Conversion_Extension should be applicable for fixed point types. I think block check where we assign ICK_Fixed_Point_Conversion should make sure we don't add any other SCSs.

@PiJoules PiJoules force-pushed the extend-fixed-point-semantics-to-c++ branch 3 times, most recently from 28dc7fe to f0e6a03 Compare November 16, 2023 00:33
@PiJoules PiJoules force-pushed the extend-fixed-point-semantics-to-c++ branch from f0e6a03 to 59582cb Compare November 16, 2023 00:34
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@PiJoules PiJoules merged commit b2d62c9 into llvm:main Nov 16, 2023
2 of 3 checks passed
@PiJoules PiJoules deleted the extend-fixed-point-semantics-to-c++ branch November 16, 2023 21:11
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants