This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by codemzs on Apr 30 2023, 11:41 PM.

Details

Summary

This commit implements Core language changes based on P1467R9 Extended floating-point types and standard names

As per the proposal's definition the following two types are marked as extended floating point: Float16 (aka _Float16) and Bfloat16 (aka decltype (0.0bf16) or __bf16). Future work can extend this to support other floating-point types such as Float32, Float64, and Float128. Please refer to the RFC for more details.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: njames93. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
codemzs requested review of this revision.Apr 30 2023, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2023, 11:41 PM

I don't believe that there is NativeBFloat16Type. AArch64 learned bf16 with ARMv8.6-A, but very limited operations and only in SIMD. X86 supports bf16 since Cooperlake, but very limited operations and only in SIMD.

Maybe GPUs?

codemzs added a comment.EditedMay 1 2023, 12:27 AM

I don't believe that there is NativeBFloat16Type. AArch64 learned bf16 with ARMv8.6-A, but very limited operations and only in SIMD. X86 supports bf16 since Cooperlake, but very limited operations and only in SIMD.

Maybe GPUs?

Dear @tschuett,

Thank you for your input. It seems that NativeBFloat16Type is indeed limited in availability. As you mentioned, AArch64 and X86 offer some bf16 support, but only in SIMD and with limited operations. This flag is intended to guard changes for targets with full bf16 support, as discussed in this RFC: https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033/12.

codemzs added inline comments.May 1 2023, 6:55 AM
libcxxabi/test/test_demangle.pass.cpp
10921 ↗(On Diff #518405)

Forgot to update the mangled name as these tests don't seem to run locally with cmake check-all

codemzs updated this revision to Diff 518433.May 1 2023, 7:11 AM

Updated test_demangle.pass.cpp with mangled name for clang::Type::isArithmeticType(clang::ASTContext&) const, didn't catch this while testing locally using cmake check-all.

codemzs set the repository for this revision to rG LLVM Github Monorepo.May 1 2023, 8:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 1 2023, 8:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
codemzs updated this revision to Diff 518468.May 1 2023, 9:18 AM

Fix Clang format failures.

codemzs added a comment.EditedMay 1 2023, 9:27 AM

My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running clang-format on it changes bunch of lines.

philnik added a subscriber: philnik.May 1 2023, 1:00 PM

My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running clang-format on it changes bunch of lines.

Please don't clang-format the test then. There is no need to do it, and the changes look rather confusing. I don't really understand why you add it at all TBH. This doesn't look like it tests anything from this patch.

codemzs updated this revision to Diff 518589.May 1 2023, 4:08 PM
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Revert the changes to /llvm-project/libcxxabi/test/test_demangle.pass.cpp. Updating the function prototype for clang::Type::isArithmeticType() const causes clang-format issues and leads to numerous unnecessary line changes. As this specific test file isn't crucial for testing the current patch, it's acceptable to leave it unchanged.

My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running clang-format on it changes bunch of lines.

Please don't clang-format the test then. There is no need to do it, and the changes look rather confusing. I don't really understand why you add it at all TBH. This doesn't look like it tests anything from this patch.

@philnik Thank you for your feedback on the changes to libcxxabi/test/test_demangle.pass.cpp. I apologize for any confusion my initial approach may have caused. I intended to keep the test file consistent with the updated function prototype, but I understand your concern about clang-format's impact on readability.

I will revert the changes to the test file as you suggested, ensuring it does not affect the current patch's functionality. I appreciate your guidance and will consider these aspects in future updates.

philnik removed a reviewer: Restricted Project.May 1 2023, 4:51 PM

My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running clang-format on it changes bunch of lines.

Please don't clang-format the test then. There is no need to do it, and the changes look rather confusing. I don't really understand why you add it at all TBH. This doesn't look like it tests anything from this patch.

@philnik Thank you for your feedback on the changes to libcxxabi/test/test_demangle.pass.cpp. I apologize for any confusion my initial approach may have caused. I intended to keep the test file consistent with the updated function prototype, but I understand your concern about clang-format's impact on readability.

I will revert the changes to the test file as you suggested, ensuring it does not affect the current patch's functionality. I appreciate your guidance and will consider these aspects in future updates.

No worries :D. The clang-format guidelines are a bit confusing, and there is nothing wrong with not knowing all the conventions of the different subprojects.

I

clang/include/clang/AST/ASTContext.h
27

What is this needed for?

2819
2820
2822
clang/include/clang/Basic/DiagnosticSemaKinds.td
8931
clang/include/clang/Basic/LangOptions.def
468 ↗(On Diff #518589)

I'm not sure I like the implications or maintainability of this. This is something that is likely to get out of hand too quickly. We probably need to just figure out what the itanium mangling is GOING to be (perhaps @rjmccall has some insight?), and just implement that.

Also, this isn't really a 'Language Option', so much as a codegen option.

clang/lib/AST/ASTContext.cpp
116

I don't quite see what this is used for yet, but nested unordered-maps for what is all compile-time constants seems like a waste of compile-time. I wonder if there is a good way to use a table for this comparison (perhaps with some level of adjustment on the type-kinds to make them non-sparse).

7110

by coding standard, you can't use 'auto' here. Also, variables are capital. I probably just would return it without the intermediary variable.

That said, this is exactly what I feared the unordered-map table above was for. We need to come up with a better storage medium for that.

My thought is to introduce something like:

constexpr unsigned FloatRankToIndex(clang::BuiltinType::Kind) {
  switch(Kind) {
    case clang::BuiltinType::Float16: return 0;  
    case clang::BuiltinType::BF16: return 1;
    case clang::BuiltinType::Float: return 2;
    case clang::BuiltinType::Double: return 3;
    case clang::BuiltinType::LongDouble: return 4;
    default: llvm_unreachable("Not a floating builtin type");
 }

And just run that on LHSKind and RHSKind here. Then, the CXX23FloatingPointConversionRankMap can be just a 2d pair of arrays:

std::array<std::array<FloatingRankCompareResult, 5>, 5> CXX23FloatingPointConversionRankMap =

We get the same results with very small runtime cost (for the 2 switches per operation), but save two unordered_map lookups, AND save the runtime construction of the unordered_maps.

clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp
5 ↗(On Diff #518589)

entry isn't a stable name here, so you shouldn't use this label.

101 ↗(On Diff #518589)

I'd vastly prefer these check-next commands be interlaced in teh code, so it is easy to see which line is supposed to associate with which.

clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp
9 ↗(On Diff #518589)

How we emit the result of the floats is inconsistent, at least in IR, so I'm not sure how stable it'll be here.

Also, don't use the |- and | \-`` in the check-lines, those get messy when something changes.

466 ↗(On Diff #518589)

Definitely need the newline here.

codemzs marked 10 inline comments as done.May 2 2023, 11:45 PM

Addressing @erichkeane 's review comments and pushing out the updated patch.

clang/include/clang/Basic/LangOptions.def
468 ↗(On Diff #518589)

Thank you for your valuable feedback. Based on your suggestion and considering that GCC has already implemented the mangling as DF16b, I agree that if we have reasonable confidence in the direction the mangling will take, it would be better to implement it directly instead of guarding it with an experimental flag. Therefore, I will be removing the flag. I initially added the flag since @tahonermann was on board with implementing the bf16 arithmetic type, assuming the mangling was finalized.

However, I understand your concerns and would appreciate further input from both @tahonermann and @rjmccall on this matter. My intention is to avoid stalling the progress of this change due to mangling finalization.

I'm open to further discussion and collaboration to ensure we make the right decision for our project while maintaining the momentum of the review process.

clang/lib/AST/ASTContext.cpp
7110

Thank you for your insightful suggestion! I agree that using a 2D array along with the FloatRankToIndex function is a more efficient and cleaner approach. I've updated the implementation accordingly. I appreciate your guidance on this.

clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp
5 ↗(On Diff #518589)

I have removed the "entry" label as you suggested.

Just to let you know, this label was automatically generated by the script utils/update_cc_test_checks.py, which is used to update the expected output of the test cases. I noticed that this label is present in other tests in the codebase as well.

Out of curiosity, why is this label not considered stable?

101 ↗(On Diff #518589)

I understand your preference for interlacing CHECK-NEXT commands with the code to improve readability. Unfortunately, the update_cc_test_checks.py script doesn't seem have an option to interlace CHECK-NEXT statements, and it can be challenging to achieve this in certain scenarios, such as when we have multiple loads followed by stores.

To address your concern, I've refactored the test code into smaller functions, which should make it easier to read and understand the relationship between the code and the corresponding CHECK statements. I hope this improves the readability and addresses your concerns. If you have any further suggestions or improvements, please don't hesitate to let me know. I'm open to making any necessary changes to ensure the code is clear and maintainable.

clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp
9 ↗(On Diff #518589)

I've observed similar floating-point representations in other tests, but I acknowledge your concerns about potential inconsistencies and stability. To address this, would replacing them with {{.*}} be a suitable solution? I'm also considering dividing the test into two parts: one for error checks in the current location and another for AST checks under test/AST. Would this resolve your concerns about the use of |- and | characters? Furthermore, I'd like to clarify whether your comment about avoiding |- applies specifically to tests in the test/Sema directory or more generally. My intention is to seek clarification and ensure the test code adheres to best practices.

codemzs updated this revision to Diff 518984.May 2 2023, 11:46 PM
codemzs marked 3 inline comments as done.

Addressing @erichkeane 's code review comments.

codemzs marked an inline comment as not done.May 2 2023, 11:51 PM

Others definitely need to review this, but i'm about happy with it.

clang/include/clang/Basic/LangOptions.def
468 ↗(On Diff #518589)

Thanks, I think this is a good direction for this patch to take.

clang/lib/AST/ASTContext.cpp
135

Just a suggestion here, feel free to toss it, but I suspect some comments to explain the 'grid' a bit are helpful.

Basically, split it up into the 5 groups, and comment which is which.

clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp
5 ↗(On Diff #518589)

Urgh, I kinda hate that script, it always gives such difficult to read tests...

My understanding is that when compiled with 'strip symbol names', no names are included that aren't required for ABI purposes. So labels all switch to %0, param names are removed entirely/etc.

Though, I guess I haven't seen that bot complain in a while, so perhaps it disappeared...

clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp
9 ↗(On Diff #518589)

I looked it up too, and I don't see this being a problem in our AST output as it is with our LLVM output, so perhaps I'm incorrect here. Feel free to leave them alone as is...

As far as the |- characters: I'd suggest just removing them entirely. Just left-align so it'd be :

VarDecl {{.*}} f16_val_6 '_Float16' cinit
FloatingLiteral {{.*}} '_Float16' 1.000000e+00

It'll still pass, and not be sensitive to the AST-dump's way of outputting.

codemzs updated this revision to Diff 519351.May 3 2023, 9:37 PM
codemzs marked 3 inline comments as done.
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Addressing @erichkeane 's code review feedback.

Thank you @erichkeane for your insightful review. I have addressed the feedback from your previous review.

codemzs updated this revision to Diff 520010.May 5 2023, 6:18 PM

Rebase to the latest from upstream as LangOpt flag for C++23 has been updated to C++23 (instead of C++2b).

codemzs marked an inline comment as done.May 5 2023, 6:26 PM
codemzs marked 2 inline comments as done.May 9 2023, 1:52 PM
codemzs added inline comments.
clang/include/clang/Basic/LangOptions.def
468 ↗(On Diff #518589)

@erichkeane @tahonermann Itanium mangling is now finalized as DF16band [[ https://github.com/itanium-cxx-abi/cxx-abi/pull/147#issuecomment-1540692344 | merged ]]into the github repository.

codemzs marked an inline comment as done.May 9 2023, 4:09 PM
This comment was removed by codemzs.

Hi @rjmccall and @erichkeane,

I would like to inquire about the necessity of implementing excess precision support for std::bfloat16_t in this patch. In reference to the discussion on https://reviews.llvm.org/D136919, it was mentioned that introducing std::bfloat16_t in Clang requires proper mangling, semantics, and excess precision support.

However, this change includes a front-end flag that enables bfloat16 arithmetic only when the target natively supports bfloat16 operations. Given this restriction, is it still essential to provide excess precision support for std::bfloat16_t in this patch?

Thank you for your guidance.

tahonermann requested changes to this revision.May 10 2023, 10:14 AM

I reviewed about a third of this, but then stopped due to the __bf16 vs std::bfloat16_t naming issues. I think the existing names that use "bfloat16" to support the __bf16 type should be renamed, in a separate patch, and this patch rebased on top of it. We are sure to make mistakes if this confusing situation is not resolved.

clang/include/clang/AST/ASTContext.h
1117–1119

This seems wrong. C++23 introduces std​::​bfloat16_t and Clang already supports __bf16. It seems to me that the BFloat16Ty name should be used for the former and BF16Ty used for the latter. I get that the inconsistency is preexisting, but I think we should fix that (in another patch before landing this one; there aren't very many references).

clang/include/clang/AST/BuiltinTypes.def
215–219 ↗(On Diff #520010)

Here again, the type names are the exact opposite of what one would intuitively expect (I do appreciate that the comment makes the intent clear, but it still looks like a copy/paste error or similar).

clang/include/clang/AST/Type.h
2241–2251

Precedent elsewhere in this file is to place multi-line comments ahead of the function they appertain to.

7372–7376

This is a great example of BFloat16 vs BF16 confusion; here the names are *not* reversed. It is really hard to know if this code is wrong or for callers to know which one they should use.

clang/include/clang/Driver/Options.td
6418–6420 ↗(On Diff #520010)

Since this message is specific to C++ for now (pending the addition of a _BFloat16 type in some future C standard, I think the message should reference std::bfloat16_t and skip explicit mention of C++23. I know it is kind of awkward to refer to the standard library name for the type, but since WG21 decided not to provide a keyword name (I wish they would just use the C names for these and get over it; they can still provide pretty library names!), there isn't another more explicit name to use. Alternatively, we could say something like "Enable the underlying type of std::bfloat16_t in C++ ...".

clang/include/clang/Lex/LiteralSupport.h
84

Is this for __bf16 or for std::bfloat16_t?

This revision now requires changes to proceed.May 10 2023, 10:14 AM
codemzs marked an inline comment as done.EditedMay 10 2023, 10:36 AM

I reviewed about a third of this, but then stopped due to the __bf16 vs std::bfloat16_t naming issues. I think the existing names that use "bfloat16" to support the __bf16 type should be renamed, in a separate patch, and this patch rebased on top of it. We are sure to make mistakes if this confusing situation is not resolved.

@tahonermann, thank you for your review and highlighting the naming issues with __bf16 and std::bfloat16_t. I agree that reversing the type names will improve readability and maintainability. I considered this while working on the code and appreciate your suggestion to address it in a separate patch before rebasing this one. I have the made change as a new patch here: https://reviews.llvm.org/D150291

clang/include/clang/Lex/LiteralSupport.h
84

Its for std::bfloat16_t, I don't believe __bf16 has a literal suffix.

stuij added a subscriber: stuij.May 12 2023, 5:40 AM
stuij added a comment.May 12 2023, 5:40 AM

I made a comment on the RFC to understand if we really need/want a new bfloat16 type.

codemzs marked an inline comment as done.May 12 2023, 9:35 AM

I made a comment on the RFC to understand if we really need/want a new bfloat16 type.

Thank you, @stuij, for your input. I will continue to follow the consensus on the RFC regarding the introduction of a new bfloat16 type. For context, my initial implementation transitioned the existing storage-only __bf16 type into an arithmetic type. If the decision leans towards not introducing a new bfloat16 type, I'm prepared to revert my changes to utilize the __bf16 type. To ensure our collective efforts are effectively streamlined and avoid any potential duplication, I am committed to aligning my work with the community consensus and ongoing discussions.

@tahonermann, when you have a moment, your guidance would be greatly appreciated.

codemzs updated this revision to Diff 522071.May 15 2023, 12:41 AM
codemzs retitled this revision from [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names and introduce Bfloat16 arithmetic type. to [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names and adapt __bf16 to be arithmetic type.
codemzs edited the summary of this revision. (Show Details)
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Following the RFC discussion, __bf16 has been adapted to function as an arithmetic type where hardware natively supports bfloat16 arithmetic. @stuij, as you initially introduced __bf16 as a storage-type, your review on this adjustment would be greatly appreciated.

codemzs edited the summary of this revision. (Show Details)May 15 2023, 12:42 AM
codemzs updated this revision to Diff 523135.May 17 2023, 12:29 PM
codemzs marked 3 inline comments as done.

Addresses code style feedback from @tahonermann

I have created a separate change to upgrade existing __bf16 to arithmetic type at D150913

clang/lib/AST/Type.cpp
2249

Remove

clang/lib/Basic/Targets/AMDGPU.h
122 ↗(On Diff #522071)

Remove

stuij added a comment.May 22 2023, 1:40 AM

@stuij, as you initially introduced __bf16 as a storage-type, your review on this adjustment would be greatly appreciated.

Sorry for the late reaction, was on holiday. Yes, I'll have a look. I've put it on my todo.

codemzs updated this revision to Diff 526519.May 29 2023, 11:34 PM
codemzs retitled this revision from [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names and adapt __bf16 to be arithmetic type to [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names.
codemzs edited the summary of this revision. (Show Details)
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Hi @tahonermann / @erichkeane ,

This change has been simplified and rebased onto D150913, which is now in LLVM main. I hope this makes the review process more straightforward.

CC: @rjmccall / @pengfei

codemzs updated this revision to Diff 526524.May 29 2023, 11:45 PM

Remove unused header file.

stuij added a comment.Jun 2 2023, 7:37 AM

This is going to be a very unhelpful comment. After looking through the changes, I don't have any comments to make, but I also don't feel comfortable to accept this revision as I don't feel to know enough about the front-end.

This is going to be a very unhelpful comment. After looking through the changes, I don't have any comments to make, but I also don't feel comfortable to accept this revision as I don't feel to know enough about the front-end.

@stuij, I sincerely appreciate you taking the time to review the changes. Your hesitation due to unfamiliarity with the front-end elements is completely understandable, and I respect your candid feedback.

@erichkeane, given your extensive contributions to the core Sema* files, I believe your expertise and experience would be particularly valuable in reviewing the changes I've made. I recall your initial informal approval for the change, and since then, I've further refined it after incorporating the outcomes of D150913. I'd be most appreciative if you could please review this revision once again.

My intention is to ensure this revision aligns with our shared vision for LLVM/Clang, and your reviews will greatly contribute to this goal. If there are any other changes or improvements required for the successful landing of this revision, please feel free to let me know.

This is going to be a very unhelpful comment. After looking through the changes, I don't have any comments to make, but I also don't feel comfortable to accept this revision as I don't feel to know enough about the front-end.

@stuij, I sincerely appreciate you taking the time to review the changes. Your hesitation due to unfamiliarity with the front-end elements is completely understandable, and I respect your candid feedback.

@erichkeane, given your extensive contributions to the core Sema* files, I believe your expertise and experience would be particularly valuable in reviewing the changes I've made. I recall your initial informal approval for the change, and since then, I've further refined it after incorporating the outcomes of D150913. I'd be most appreciative if you could please review this revision once again.

My intention is to ensure this revision aligns with our shared vision for LLVM/Clang, and your reviews will greatly contribute to this goal. If there are any other changes or improvements required for the successful landing of this revision, please feel free to let me know.

I'll put you on my list to re-review for early next week, though Aaron probably needs to do a look through this as well.

This is going to be a very unhelpful comment. After looking through the changes, I don't have any comments to make, but I also don't feel comfortable to accept this revision as I don't feel to know enough about the front-end.

@stuij, I sincerely appreciate you taking the time to review the changes. Your hesitation due to unfamiliarity with the front-end elements is completely understandable, and I respect your candid feedback.

@erichkeane, given your extensive contributions to the core Sema* files, I believe your expertise and experience would be particularly valuable in reviewing the changes I've made. I recall your initial informal approval for the change, and since then, I've further refined it after incorporating the outcomes of D150913. I'd be most appreciative if you could please review this revision once again.

My intention is to ensure this revision aligns with our shared vision for LLVM/Clang, and your reviews will greatly contribute to this goal. If there are any other changes or improvements required for the successful landing of this revision, please feel free to let me know.

I'll put you on my list to re-review for early next week, though Aaron probably needs to do a look through this as well.

Thank you, @erichkeane that would be great, are you referring to @aaron.ballman ?

This is going to be a very unhelpful comment. After looking through the changes, I don't have any comments to make, but I also don't feel comfortable to accept this revision as I don't feel to know enough about the front-end.

@stuij, I sincerely appreciate you taking the time to review the changes. Your hesitation due to unfamiliarity with the front-end elements is completely understandable, and I respect your candid feedback.

@erichkeane, given your extensive contributions to the core Sema* files, I believe your expertise and experience would be particularly valuable in reviewing the changes I've made. I recall your initial informal approval for the change, and since then, I've further refined it after incorporating the outcomes of D150913. I'd be most appreciative if you could please review this revision once again.

My intention is to ensure this revision aligns with our shared vision for LLVM/Clang, and your reviews will greatly contribute to this goal. If there are any other changes or improvements required for the successful landing of this revision, please feel free to let me know.

I'll put you on my list to re-review for early next week, though Aaron probably needs to do a look through this as well.

Thank you, @erichkeane that would be great, are you referring to @aaron.ballman ?

Yes.

codemzs removed a reviewer: asmith.Jun 4 2023, 11:30 AM
codemzs added a reviewer: rjmccall.

This all LGTM, but I want @tahonermann to do a pass as well, he knows more about FP than I do, so I don't want to accept without his run-through.

This all LGTM, but I want @tahonermann to do a pass as well, he knows more about FP than I do, so I don't want to accept without his run-through.

Thank you, @erichkeane for the review. @aaron.ballman if you can please also have a look when you get a chance it would be great!

@tahonermann Just a quick note - I've made the changes you initially suggested in the RFC. When you get a chance, could you have another look?

@tahonermann Just a quick note - I've made the changes you initially suggested in the RFC. When you get a chance, could you have another look?

Yes, I hope to today. If not, I'll make it a priority for tomorrow.

This is looking good to me. I added a few comments seeking clarification.

I'm concerned about the changes to the existing calls to getFloatingTypeOrder(). The changes look like they will preserve prior behavior for standard types just fine, but I'm not sure whether they implement the right behavior for the extended types. Defining comparison functions that are explicit in their handling of FRCR_Unordered and subrank comparisons would help me feel more confident about the changes, the intent, and my ability to audit for the desired behavior.

clang/include/clang/Lex/LiteralSupport.h
84

I guess it does now! :)

clang/lib/AST/ASTContext.cpp
127–128

Assuming there is no need to handle __float128 and __ibm128 here, how about a comment to state why they don't require support here?

clang/lib/Sema/Sema.cpp
711

This comparison depends on the order of the enumerators in FloatingRankCompareResult and that seems brittle. I suggest either encompassing such comparisons in a function or, at least, adding a comment to the enumeration definition that explains the ordering requirements.

clang/lib/Sema/SemaCast.cpp
1394

Should this be a FIXME comment? What happens in this case? Should we perhaps assert on such attempted conversions?

clang/lib/Sema/SemaExpr.cpp
1247–1249

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?

clang/lib/Sema/SemaOverload.cpp
4329

This comparison includes FRCR_Unordered, is that intended? (another case at line 4225 below)

codemzs updated this revision to Diff 529171.Jun 6 2023, 11:10 PM
codemzs marked 4 inline comments as done.
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Hi @tahonermann,

Thank you for your insights on the patch. I concur with your perspective about the potential brittleness of comparison operations on the FloatingRankCompareResult enum type. As a remedy, I have incorporated a helper routine to handle such operations.

Regarding the getFloatingTypeOrder()method, it employs a 2-D grid to evaluate the conversion rank between two types. The grid index for a type is firmly established in the CXX23FloatRankToIndex, offering a stable indexing mechanism. Hence, it appears that this approach mitigates the brittleness concern associated with the comparison of FloatingRankCompareResult enum values.

I'm open to further insights if there are aspects I may have overlooked.

codemzs added inline comments.Jun 6 2023, 11:12 PM
clang/lib/Sema/SemaCast.cpp
1394

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.

clang/lib/Sema/SemaExpr.cpp
1247–1249

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.

clang/lib/Sema/SemaOverload.cpp
4329

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?

@tahonermann Gentle ping, please let me know if you have any questions.

@tahonermann Gentle ping, please let me know if you have any questions.

FWIW, there are C++ standards meetings all week this week, and C standards meetings all next week (and into the following week), so @tahonermann (and other reviewers) might be slower to respond as a result.

tahonermann requested changes to this revision.Jun 23 2023, 9:13 PM

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.

clang/lib/AST/ASTContext.cpp
192

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.

7091–7095
clang/lib/Frontend/InitPreprocessor.cpp
460

Should the definition of __STDCPP_BFLOAT16_T__ be conditional on something like Ctx.getTargetInfo().hasFullBFloat16Type()?

clang/lib/Sema/SemaCast.cpp
1394

If the condition can be legitimately hit, then we definitely want error handling rather than an assert. Thanks, this looks good.

clang/lib/Sema/SemaChecking.cpp
11749

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.

15488–15515

Should FRCR_Unordered be handled in some way here? It seems like we should at least assert that the types are not unordered.

clang/lib/Sema/SemaExpr.cpp
1247–1249

Ah, thank you. This looks good then.

1256–1259

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.

clang/lib/Sema/SemaOverload.cpp
4329

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.

clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
4–7

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.

This revision now requires changes to proceed.Jun 23 2023, 9:13 PM
codemzs updated this revision to Diff 546100.Aug 1 2023, 9:45 AM
codemzs marked an inline comment as done.

Updated with feedback from @tahonermann

@tahonermann I would like to understand your concern better on unordered floating point types as the callers of getFloatingTypeOrder handle this result as per the C++23 proposal, for example there is a test case that exercises this scenario: _Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}

Perhaps if you can please give me an example that further illustrates your concerns it would help me understand and assuage your concerns. Thank you.

clang/lib/AST/ASTContext.cpp
192

That is correct, my plan was to add support for these types after this change is committed.

clang/lib/Frontend/InitPreprocessor.cpp
460

Based on our discussion on the RFC it was determined that representing bfloat16 using fp32 is ok as per the author of the proposal, in that case do you believe we should be making this macro conditional on the target natively supporting bfloat16?

clang/lib/Sema/SemaChecking.cpp
11749

Rules for promotion are unchanged as per the proposal. This is just using the new enumeration to represent a smaller conversion rank.

clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
4–7

I believe this should be an error unless my understanding is incorrect, below is what the proposal says for constant expressions:

"A drawback of this part of the proposal is that constant values don’t get any special treatment. As a result, this code:

std::float16_t x = 1.0;

would be ill-formed. The constant 1.0 has type double, which cannot be implicitly converted to type std::float16_t, even though the value 1.0 can be represented exactly in both types. To compile, the code must have an explicit cast, or, preferably, use a literal suffix:
std::float16_t x = 1.0f16;
...."

@codemzs, I'm sorry for the very long delay following up on this review. I've been struggling to keep up, but expect to be able to devote some time to this next week. I'm committed to helping to ensure we land this before Phabricator stops accepting new diffs (proposed for November 15th).

codemzs updated this revision to Diff 558083.Nov 13 2023, 10:46 AM
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Synced to the main and resolved merge conflicts, updated tests.

codemzs updated this revision to Diff 558099.Nov 14 2023, 2:53 PM

Hi @tahonermann,

I've just pushed a new diff with tests for va_arg and ..., ensuring promotion rules are intact.

Also, I've made sure getFloatingTypeOrder returns FRCR_Unordered only when we're dealing with both operands as C++23 extended floating point types and they are unordered. I've added tests like _Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}} to cover these cases. If you see any potential issues here, I'm all ears and ready to make adjustments.

Thanks a lot for reviewing this, especially with your busy schedule.

Apologies once again for the delayed response. I reviewed some today and will resume reviewing on Monday.

In addition to the inline suggestions:

clang/docs/ReleaseNotes.rst will need to be updated to reflect that the core language changes for P1467R9 have been implemented for std::float16_t and std::bfloat16_t.

Given the confusing array of 16-bit floating-point types, how about modifying clang/include/clang/AST/BuiltinTypes.def to be more explicit regarding which is which?

// 'half' in OpenCL, '__fp16' in ARM NEON.
FLOATING_TYPE(Half, HalfTy)
...
// '_Float16', 'std::float16_t'
FLOATING_TYPE(Float16, HalfTy)

// '__bf16', 'std::bfloat16_t'
FLOATING_TYPE(BFloat16, BFloat16Ty)

Hmm, having pasted the above, I now noticed that the Half and Float16 types share the HalfTy singleton. Unless I'm mistaken, the changes being made will cause divergent behavior. Do these now need to be split?

clang/include/clang/AST/ASTContext.h
56

Naming suggestion: FloatConvRankCompareResult.

1119

I think the comment should reference http://eel.is/c++draft/basic.extended.fp#1 for std::float16_t. p5 is for std::bfloat16_t.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8931

The diagnostic text here looks more like the text of a warning. Since this is an error, I think it makes more sense to model the text on other similar errors and use "narrowing" or "implicit cast" terminology. For examples, see err_spaceship_argument_narrowing and err_impcast_complex_scalar

Additionally, it would be helpful to include the relevant types in the message.

Finally, line length should be kept to 80 characters or less per https://llvm.org/docs/CodingStandards.html#source-code-width.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
779–788

This looks like a pre-existing bug since, for standard floating-point types, narrowing implicit conversions are allowed. I think the intent was to add a cast on which ever side is needed, but the existing code instead adds a cast on the RHS when the LHS has a higher rank, adds a cast on the LHS when the LHS and RHS have the same rank, and wanders into UB when the RHS has a higher rank.

The incorrect comparison goes back to when the code was introduced in commit 08f943c5630d8ee31d6a93227171d2f05db59e62.

The introduction of unordered conversion ranks suggests additional changes are needed here, but it isn't clear to me what the right changes are. I added a suggested edit that adds an arbitrary cast (which probably suffices for the purposes of static analysis). Alternatively, an assert could be added for the unordered and equal cases.

clang/lib/Sema/SemaChecking.cpp
11749

I think this still produces an incorrect result in some cases though. According to http://eel.is/c++draft/conv.fpprom, the only floating-point promotion is from float to double.

Ah, I think the prior code was incorrect, but non-symptomatic because it is only currently used in one place (in CheckPrintfHandler::checkFormatExpr()) and that code only cares about the integer cases. I would prefer that we fix this rather than extend the issue into extended FP types. One way to fix it would be to introduce isPromotableFloatingType() and getPromotedFloatingType() functions to match the existing ones integers used above.

Just starting to look at this. Don't we need a RN for this?

So while trying to review this patch, I've discovered there's an annoying incompatibility between C and C++ here, in that C and C++ specify different rules on how to choose between _Float64, double, and long double if all are binary64 (C says _Float64 > long double > double, C++ says long double > _Float64 > double), and I don't have enough knowledge of clang internal implementation to know if the changes being done can capture the different semantics there. (It's also not entirely clear to me that the incompatibility between C and C++ was intentional in the first place; it looks like the paper author intentionally chose the ordering for C++ and argued for the change to C, but the CFP working group soundly rejected the change, and personally I agree with the CFP decision here over C++).

clang/lib/AST/ASTContext.cpp
136–138

I don't like having a large table here of results. It just doesn't scale; if you take into account all of the putative types that might be supported, you've got 3 basic types + 4 interchange formats + 3 decimal types + 1 IEEE extended type + bfloat + 3 IBM hex floats, and that's just things already supported by LLVM or that I know someone's working on.

I think after special casing float/double/long double, you can handle all the other types by simply using a mixture of fltSemantics::isRepresentableBy and a subrank preference list.

In the event that support for _Float32 and _Float64 are added, this table will no longer be target-independent. We have a few targets where double is binary32 and several targets where long double is either binary32 or binary64 (and others where it's neither). I think it's better to start writing this method in a way that can handle this mapping be target-dependent.