Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[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

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
1111–1113

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
2165–2175

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

7276–7280

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
75

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
75

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

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
2235

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
75

I guess it does now! :)

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

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
705

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
1386

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

clang/lib/Sema/SemaExpr.cpp
1246–1248

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
4204

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
1386

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
1246–1248

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
4204

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
191

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.

7137–7141
clang/lib/Frontend/InitPreprocessor.cpp
461

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

clang/lib/Sema/SemaCast.cpp
1386

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
11085

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.

14821–14848

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
1246–1248

Ah, thank you. This looks good then.

1255–1258

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
4204

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
191

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

clang/lib/Frontend/InitPreprocessor.cpp
461

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
11085

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