[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.

Sorry for the long delay reviewing again. I noted some mostly minor issues. I'm on vacation next week, but I'll try to watch for updates.

I'm still concerned that the changes to the code that was already performing rank comparisons don't explicitly do anything for unordered cases. I think I would feel better if we at least asserted that the comparisons are not unordered.

I'd like to see tests added to exercise `va_arg` and passing the extended types through `...`; this will exercise the promotion related changes.



================
Comment at: clang/lib/AST/ASTContext.cpp:191
+           FloatingRankCompareResult::FRCR_Greater,
+           FloatingRankCompareResult::FRCR_Equal}}}};
+
----------------
I just realized that none of these comparisons results in `FRCR_Equal_Lesser_Subrank` or `FRCR_Equal_Greater_Subrank`. I guess those won't actually be used until support for `std::float32_t` and `std::float64_t` are added. That means that we don't have a way to test code that performs subrank checks though.


================
Comment at: clang/lib/AST/ASTContext.cpp:7141-7145
+/// If LHS > RHS, return FRCR_Greater.  If LHS == RHS, return FRCR_Equal. If
+/// LHS < RHS, return FRCR_Lesser. If the values representedable by the two
+/// are not subset of each other, return FRCR_Unordered. If LHS == RHS but
+/// LHS has a higher subrank than RHS return FRCR_Equal_Greater_Subrank else
+/// return FRCR_Equal_Lesser_Subrank.
----------------



================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:461
+      Builder.defineMacro("__STDCPP_FLOAT16_T__", "1");
+      Builder.defineMacro("__STDCPP_BFLOAT16_T__", "1");
+    }
----------------
Should the definition of `__STDCPP_BFLOAT16_T__` be conditional on something like `Ctx.getTargetInfo().hasFullBFloat16Type()`?


================
Comment at: clang/lib/Sema/SemaCast.cpp:1367
+      Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+    // Support for cast between fp16 and bf16 doesn't exist yet.
+    if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&
----------------
codemzs wrote:
> tahonermann wrote:
> > Should this be a FIXME comment? What happens in this case? Should we perhaps assert on such attempted conversions?
> I have added the FIXME. There is a test case for this scenario:
> 
> ```_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error {{static_cast from '__bf16' to '_Float16' is not allowed}}```
> 
> If you still believe it might be better to add an assert I can do that, please let me know.
If the condition can be legitimately hit, then we definitely want error handling rather than an assert. Thanks, this looks good.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
-         S.Context.getFloatingTypeOrder(From, To) < 0;
+         S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }
----------------
I'm not sure this is right. If I understand correctly, the C++23 extended FP types don't participate in argument promotions. Perhaps they should be excluded here.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:14170-14195
       int Order = S.getASTContext().getFloatingTypeSemanticOrder(
           QualType(SourceBT, 0), QualType(TargetBT, 0));
-      if (Order > 0) {
+      if (Order == FRCR_Greater) {
         // Don't warn about float constants that are precisely
         // representable in the target type.
         Expr::EvalResult result;
         if (E->EvaluateAsRValue(result, S.Context)) {
----------------
Should `FRCR_Unordered` be handled in some way here? It seems like we should at least assert that the types are not unordered.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1244-1246
+    assert(((order != FRCR_Equal) &&
+            (order == FRCR_Lesser || order == FRCR_Equal_Lesser_Subrank)) &&
+           "illegal float comparison");
----------------
Since the assertion requires `order` to be one of `FRCR_Lesser` or `FRCE_Equal_Lesser_Subrank`, there is no need to check for `FRCR_Equal`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+    if (order == FRCR_Unordered) {
+      return QualType();
+    }
----------------
codemzs wrote:
> tahonermann wrote:
> > Return of an invalid type is a new behavior for this function and it isn't clear to me that callers will handle it (or be expected to handle it) such that a diagnostic will be generated. Perhaps a diagnostic should be issued here? Or perhaps this should be an assertion?
> It results in an error, please see the below test case:
> 
> ```auto f16_bf16 = 1.0f16 + 1.0bf16; // expected-error {{invalid operands to binary expression ('_Float16' and '__bf16')}}```
> 
> This function is only called by `UsualArithmeticConversions` which returns invalid type in several places, hence returning invalid type should be ok? If you still prefer an assertion or diagnostic, I will happily add one.
Ah, thank you. This looks good then.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+          if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+                                             SCS1.getFromType()) < FRCR_Equal) {
+            return ImplicitConversionSequence::Better;
----------------
codemzs wrote:
> tahonermann wrote:
> > This comparison includes `FRCR_Unordered`, is that intended? (another case at line 4225 below)
> Yes, my understanding is we are checking if the second sequence's type's (T3) conversion rank is not equal to the conversion rank of FP2. Is that not correct?
I definitely trust you more than myself to know if it is correct! I haven't really internalized the unordered and sub-rank concerns yet.


================
Comment at: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp:3-6
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
----------------
Are these errors correct? Since the source is a constant expression that specifies a value that is within the representable range of the type, I think these initializations are expected to be allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573