@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'}}

Updated with feedback from @tahonermann

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

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.

In D149573#4395968, @erichkeane wrote:

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.

In D149573#4391013, @erichkeane wrote:

In D149573#4390895, @codemzs wrote:

In D149573#4390863, @stuij wrote:

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.

In D149573#4390863, @stuij wrote:

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.

Closing this as it has been resolved by D150913.

Remove unused header file.

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

Fix indentation and spacing.

Thank you for your valuable review and acceptance of my patch. As I lack commit access, could I kindly request one of you to perform the commit on my behalf? Please use the following command: git commit --amend --author="M. Zeeshan Siddiqui <mzs@microsoft.com>".

Addresses @rjmccall suggestions.

Addresses feedback on extended floating type documentation from @rjmccall and @pengfei

Incorporates suggestions provided by @rjmccall, @pengfei, and @zahiraam.

@pengfei, @zahiraam, I appreciate your feedback.

I believe I had updated the __bf16 documentation in /llvm-project/clang/docs/LanguageExtensions.rst, but it appears to have been omitted in this patch. I assure you, I'll rectify this in the next iteration.

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

Misc style improvement.

Addresses code style feedback from @tahonermann

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.

In D150291#4338118, @tahonermann wrote:

I do wonder if we need two bfloat implementations, but for that I'll leave a comment on D149573.

Given the discussions occurring in D149573, let's hold off on landing this for now. It is sounding like we might have direction for repurposing __bf16 for std::bfloat16_t (and a future _BFloat16 C type; with __bf16 retained as an alternate spelling). If we go in that direction, then we would presumably want a change that goes in the opposite direction of this patch; a change that migrates "bf16" names towards "bfloat16". Let's focus on confirming that direction first. I'll mark this as requesting changes for now while we figure this out.

In D149573#4337480, @stuij wrote:

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

In D150291#4335352, @tahonermann wrote:

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But please wait a few days for recently subscribed folks to have a chance to comment before landing this.

Addressing feedback from @barannikov88

In D150291#4335360, @barannikov88 wrote:

The summary as it is will be hard to read in git log. Please split it into multiple lines 72~80 chars each.
https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

Update comments as per feedback from @tahonermann

@tahonermann Thank you for pointing that out and for reviewing my code. I appreciate your guidance. I was following the LLVM contribution guidelines to use git clang-format, but I understand the importance of maintaining existing code styles that may be altered by git-clang format. I will be more mindful of this in the future. I have reverted the style changes and updated the patch as per your feedback.

PR feedback: Revert style changes.

In D149573#4332549, @tahonermann wrote:

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.

Hi @rjmccall and @erichkeane,

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

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

Addressing @erichkeane 's code review feedback.

Addressing @erichkeane 's code review comments.

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

In D149573#4310601, @philnik wrote:

In D149573#4310009, @codemzs wrote:

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.

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.

Fix Clang format failures.

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.

In D149573#4309378, @tschuett wrote:

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?

On a second thought, it seems you have another change where you are actually fixing the bump allocator so why not check that in instead of this band-aid fix?

Will be nice to add some comments why you are relanding and why this won't cause build breaks again.

In D121638#3380884, @saugustine wrote:

Reverted with ee7a286cd3e4364d2f7d5b6ba4b8a6fc0d524854

This change has broken windows builds:

In D117637#3254195, @labath wrote:

I needed to dust off my old-deps build for another patch, so I also fixed this in the process.