Skip to content

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Aug 22, 2025

Before this patch, the application of getAs and castAs on a leaf type would always produce a canonical type, which is undesirable because some of these types can be sugared.

The user expectation is that getAs only removes top level sugar nodes, leaving all the type sugar on the returned node, but it had an optimization intended for type nodes with no sugar: for these, we can skip the expensive traversal of the top level sugar with a simple canonicalization followed by dyn_cast.

The problem is that the concept of leaf type does not map well to what is correct to apply this optimization to.

This patch replaces the concept of leaf types with 'always canonical' types, and only applies the canonicalization strategy on them.

In order to avoid the performance regression this would cause, as most current users do not care about type sugar, this patch also replaces all of these uses with alternative cast functions which operate through canonicalization.

  • Introduces castAs variants to complement the getAsTagDecl and derived variants.
  • Introduces getAsEnumDecl and castAsEnumDecl, complementing the current set, so that all TagDecls are covered.
  • Introduces getAsCanonical and castAsCanonical, for faster casting when only the canonical type is desired.

The getAsTagDecl and related functions are not provided inline, because of the circular dependencies that would involve. So this patch causes a small overall performance regression:
image

This will be fixed in a later patch, bringing the whole thing back to a positive performance improvement overall:
image

These follow-up patches are:

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Meat and Potatoes" patch looks fine to me. Rest of it is too large to look at beyond scrolling through. Would be nice to have a few other +1s on this, just because of the size.

mizvekov added a commit that referenced this pull request Aug 25, 2025
This changes a bunch of places which use getAs<TagType>, including
derived types, just to obtain the tag definition.

This is preparation for #155028, offloading all the changes that
PR used to introduce which don't depend on any new helpers.
mizvekov added a commit that referenced this pull request Aug 25, 2025
…#155313)

This changes a bunch of places which use getAs<TagType>, including
derived types, just to obtain the tag definition.

This is preparation for #155028, offloading all the changes that PR used
to introduce which don't depend on any new helpers.
mizvekov added a commit that referenced this pull request Aug 25, 2025
…ends

This is a small performance improvement:

This helps recover the performance lost in #155028, reversing it
into a small positive instead.
@mizvekov mizvekov force-pushed the users/mizvekov/improve-type-helpers branch from fe87fbc to 26b035d Compare August 25, 2025 23:41
@mizvekov
Copy link
Contributor Author

@rjmccall done, I have offloaded all of the changes this PR did which didn't need the introduction of a new helper.

That halved the size of the patch.

The "meat and potatoes" commit still contains the important changes.

mizvekov added a commit that referenced this pull request Aug 26, 2025
… TagDecls

And make use of those.

These changes are split from prior PR #155028, in order to decrease the
size of that PR and facilitate review.
mizvekov added a commit that referenced this pull request Aug 26, 2025
… TagDecls (#155463)

And make use of those.

These changes are split from prior PR #155028, in order to decrease the
size of that PR and facilitate review.
@mizvekov mizvekov force-pushed the users/mizvekov/improve-type-helpers branch from 26b035d to 745eb0b Compare August 26, 2025 19:08
@mizvekov
Copy link
Contributor Author

I did another offloading of NFC changes to another patch.

This PR is tiny now in comparison.

mizvekov added a commit that referenced this pull request Aug 26, 2025
…ends

This is a small performance improvement:

This helps recover the performance lost in #155028, reversing it
into a small positive instead.
Before this patch, the application of getAs and castAs on a leaf type
would always produce a canonical type, which is undesirable because
some of these types can be sugared.

The user expectation is that getAs only removes top level sugar nodes, leaving
all the type sugar on the returned node, but it had an optimization intended
for type nodes with no sugar: for these, we can skip the expensive traversal
of the top level sugar with a simple canonicalization followed by dyn_cast.

The problem is that the concept of leaf type does not map well to what is
correct to apply this optimization to.

This patch replaces the concept of leaf types with 'always canonical' types,
and only applies the canonicalization strategy on them.

In order to avoid the performance regression this would cause, as most current
users do not care about type sugar, this patch also replaces all of these uses
with alternative cast functions which operate through canonicalization.

* Introduces castAs variants to complement the getAsTagDecl and derived
  variants.
* Introduces getAsEnumDecl and castAsEnumDecl, complementing the current set, so
  that all TagDecls are covered.
* Introduces getAsCanonical and castAsCanonical, for faster casting when only
  the canonical type is desired.

The getAsTagDecl and related functions are not provided inline, because
of the circular dependencies that would involve. So this patch causes a
small overall performance regression:
<img width="1461" height="18" alt="image" src="https://github.com/user-attachments/assets/061dfb14-9506-4623-91ec-0f02f585d1dd" />

This will be fixed in a later patch, bringing the whole thing back to a positive
performance improvement overall:
<img width="1462" height="18" alt="image" src="https://github.com/user-attachments/assets/c237e68f-f696-44f4-acc6-a7c7ba5b0976" />
@mizvekov mizvekov force-pushed the users/mizvekov/improve-type-helpers branch from 745eb0b to 39b9487 Compare August 26, 2025 19:46
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mizvekov mizvekov merged commit 88438ba into main Aug 27, 2025
9 of 10 checks passed
@mizvekov mizvekov deleted the users/mizvekov/improve-type-helpers branch August 27, 2025 09:20
mizvekov added a commit that referenced this pull request Aug 27, 2025
…ends

This is a small performance improvement:

This helps recover the performance lost in #155028, reversing it
into a small positive instead.
mizvekov added a commit that referenced this pull request Aug 27, 2025
…ends

This is a small performance improvement:

This helps recover the performance lost in #155028, reversing it
into a small positive instead.
mizvekov added a commit that referenced this pull request Aug 27, 2025
…ends (#155051)

This is a small performance improvement:

This helps recover the performance lost in #155028, reversing it into a
small positive instead.
<img width="1464" height="20" alt="image"
src="https://github.com/user-attachments/assets/3378789e-109d-4211-846e-0d38d6cb190a"
/>
@jyknight
Copy link
Member

jyknight commented Aug 27, 2025

Heads up, this is breaking modules support in our buildsystem. I'm getting errors now like this:

libcxx/include/__atomic/memory_order.h:31:27: error: missing '#include <__atomic/memory_order.h>'; '__memory_order_underlying_t' must be declared before it is used
   31 | enum class memory_order : __memory_order_underlying_t {
      |                           ^
libcxx/include/__atomic/memory_order.h:27:7: note: declaration here is not visible
   27 | using __memory_order_underlying_t _LIBCPP_NODEBUG = __underlying_type_t<__legacy_memory_order>;

Note that the "not visible" decl is declared literally 4 lines before the usage.

I don't know how to repro this upstream (and, I'd note, we aren't using the upstream modulemap file). My suspicion is that this commit has broken module decl-merging, where multiple copies of the same header are imported.

I'll attempt to make a minimal example...

@jyknight
Copy link
Member

jyknight commented Aug 28, 2025

Minimized test-case:

cat > test.h <<EOF
#ifndef TEST_H_
#define TEST_H_
template <class _Tp>
using __underlying_type_t = __underlying_type(_Tp);
enum __legacy_memory_order { __mo_relaxed };
using __memory_order_underlying_t = __underlying_type_t<__legacy_memory_order>;
enum class memory_order : __memory_order_underlying_t { relaxed = __mo_relaxed };
#endif
EOF

cat > test1.h <<EOF
#ifndef TEST1_H_
#define TEST1_H_
#include "test.h"
#endif
EOF

cat > test2.h <<EOF
#ifndef TEST2_H_
#define TEST2_H_
#include "test.h"
#endif
EOF

cat > test.cppmap <<EOF
module "test_module" {
  export *
  module "test1.h" {
    export *
    header "test1.h"
  }
  module "test2.h" {
    export *
    header "test2.h"
  }
  textual header "test.h"
}
EOF

$CLANG -xc++ -std=gnu++20 -fmodule-name=test_module -fmodule-map-file=test.cppmap -Xclang=-emit-module -fmodules -fno-implicit-modules -fno-implicit-module-maps -c test.cppmap -o /dev/null

With a debug build of Clang, this actually results in an assert fail

clang: clang/lib/Sema/SemaType.cpp:9881: QualType GetEnumUnderlyingType(Sema &, QualType, SourceLocation): Assertion `!Underlying.isNull()' failed.

mizvekov added a commit that referenced this pull request Aug 28, 2025
Clang skips parsing a TagDecl definition in case a definition was already parsed
in another module.

In those cases, an EnumDecl might be left without an IntegerType.
Take this into account when getting the underlying type of an enum,
look for the integer type in the definition instead in those cases.

This patch also changes the implementation so it properly marks those
skipped tag definitions as demoted.

This fixes a regression reported here: #155028 (comment)

Since this regression was never released, there are no release notes.
@mizvekov
Copy link
Contributor Author

@jyknight thanks, this will be fixed here: #155900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 backend:AMDGPU backend:ARC backend:ARM backend:CSKY backend:DirectX backend:Hexagon backend:Lanai backend:loongarch backend:MIPS backend:PowerPC backend:RISC-V backend:Sparc backend:SystemZ backend:WebAssembly backend:X86 clang:analysis clang:as-a-library libclang and C++ API clang:bytecode Issues for the clang bytecode constexpr interpreter clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:openmp OpenMP related changes to Clang clang:static analyzer clang Clang issues not falling into any other category clang-tidy clang-tools-extra clangd ClangIR Anything related to the ClangIR project debuginfo HLSL HLSL Language Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants