codemzs marked 10 inline comments as done.
codemzs added a comment.
Addressing @erichkeane 's review comments and pushing out the updated patch.
================
Comment at: clang/include/clang/Basic/LangOptions.def:468
+LANGOPT(ExperimentalNewItaniumMangling, 1, 0, "experimental new Itanium
mangling")
+
----------------
erichkeane wrote:
> 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.
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.
================
Comment at: clang/lib/AST/ASTContext.cpp:7152
+ RHSKind = RHS->castAs<BuiltinType>()->getKind();
+ auto comp = CXX23FloatingPointConversionRankMap[LHSKind][RHSKind];
+ return comp;
----------------
erichkeane wrote:
> 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_map`s.
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.
================
Comment at: clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp:5
+// CHECK-LABEL: @_Z1fDF16b(
+// CHECK-NEXT: entry:
+// CHECK-NEXT: [[V_ADDR:%.*]] = alloca bfloat, align 2
----------------
erichkeane wrote:
> `entry` isn't a stable name here, so you shouldn't use this label.
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?
================
Comment at: clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp:101
+
+// CHECK-LABEL: @_Z4testv(
+// CHECK-NEXT: entry:
----------------
erichkeane wrote:
> 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.
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.
================
Comment at: clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp:9
+//CHECK: |-VarDecl {{.*}} f16_val_6 '_Float16' cinit
+//CHECK-NEXT: | `-FloatingLiteral {{.*}} '_Float16' 1.000000e+00
+_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error
{{static_cast from 'BF16' to '_Float16' is not allowed}}
----------------
erichkeane wrote:
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149573/new/
https://reviews.llvm.org/D149573
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
