In D83906#4186887, @hoy wrote:

In D83906#4184916, @dexonsmith wrote:

In D83906#4183453, @hoy wrote:

Wondering if we can come up with a way to tell the optimizer about that, e.g., through a new module flag. When it comes to LTO, the selection of linkonce_odr symbols should already been done and the optimizer may be able to recompute the attributes based on pre-LTO attributes, or at least we can allow IPO to one module only, which should still do a better job than FE does?

I don't think there's much point in passing anything to LTO. There are very few linkonce_odr symbols in LTO, since LTO has the advantage of an export list from the link. Symbols not on the export list are internalized (they're given local linkage).

That sounds to me an opportunity to get a broader IPO done precisely in the prelink optimizer, as long as we find a way to tell it the incoming IR has source fidelity. What do you think about idea of introducing a module flag? Maybe it's worth discussing in the forum as a followup of introducing a cc1 flag for a stable IR gen.

I'm not the right person to review this. @Gerolf, perhaps you can suggest an alternate?

In D83906#4183453, @hoy wrote:

Wondering if we can come up with a way to tell the optimizer about that, e.g., through a new module flag. When it comes to LTO, the selection of linkonce_odr symbols should already been done and the optimizer may be able to recompute the attributes based on pre-LTO attributes, or at least we can allow IPO to one module only, which should still do a better job than FE does?

In D83906#4182902, @rjmccall wrote:

So your argument is that it would not be possible to recognize that we're doing such an optimization and mark the function as having had a possible semantics change?

In D83906#4182847, @hoy wrote:

As far as I know, the optimizer IPO pass that infers function attributes (i..e InferFunctionAttrsPass) is placed at the very beginning of the optimization pipeline. Does this sound to you that the side effects computed for linkonce_odr functions there can be trusted by the rest of the pipeline?

In D83906#4182777, @rjmccall wrote:

In D83906#4182287, @dexonsmith wrote:

• At IRGen time, you know the LLVM attributes have not been adjusted after the optimized refined the function's behaviour. It should be safe to have IPA peepholes, as long as IRGen's other peepholes don't refine behaviour and add attributes based on that.
• In the optimizer, if you're looking at de-refineable function then you don't know which attributes come directly from the source and which were implied by optimizer refinements. You can't trust you'll get the same function attributes at runtime.

Hmm. I see what you're saying, but it's an interesting question how it applies here. In principle, the optimizer should not be changing the observable semantics of functions, which certainly includes things like whether the function throws. Maybe the optimizer can only figure out that a function throws in one TU, but if it "figures that out" and then a function with supposedly the same semantics actually does throw — not just retains the static ability to throw on a path that happens not to be taken dynamically, but actually throws at runtime — then arguably something has gone badly wrong.

In D83906#4182428, @hoy wrote:

In D83906#4182287, @dexonsmith wrote:

In C++, you get linkonce_odr all over the place. It's basically all functions that are defined in C++ headers that are available for inlining.

On the other hand, the frontend knows the token sequence from the source language. It knows whether function B is inherently nounwind based on its ODR token sequence; in which case, it's safe to use the attribute for an IPA peephole.

Thanks for the detailed explanation again! As you pointed out previously, linkonce_odr is something the front end can optimize. I'm wondering why the front end is confident about that the linker would not replace the current definition with something else.

In D83906#4181981, @hoy wrote:

That said, the LLVM optimizer does not strictly subsume the front-end because of how it fails to handle linkonce_odr functions as in https://reviews.llvm.org/D18634. I'm wondering how common the linkonce_odr linkage is for C++. In @wlei's example, none of the functions there is linkonce_odr. Is there a particular source-level annotate that specifies functions to be linkonce_odr?

Oh, de-refining is pretty nifty / evil. This patch has background:
https://reviews.llvm.org/D18634

In D83906#4179512, @wlei wrote:

Hi @ahatanak

We recently hit an issue of inconsistent codegen related with this optimization. In one build, Clang frontend generates different llvm IRs for the same function that is originally from one header file. It turned out this optimization gives different results for different function definition order which is naturally unstable.

See this two repro programs:

p1.cpp: https://godbolt.org/z/bavTYEG1x

void foo() {};
void bar() noexcept {foo();};


p2.cpp: https://godbolt.org/z/zfsnzPrE6

void foo();
void bar() noexcept {foo();};
void foo(){};

See the codegens of bar are different, for p2.cpp, the callee(foo)’s definition is after the caller(bar), it's unknown to be marked nounwind before it see foo's definition, so it still generates the invoke things.

This inconsistency affected the AutoFDO, one of our work assigns consecutive number IDs to the BBs of CFG, the unstable CFGs causes the BB ID mismatched and a lot of samples are lost.

Would like to hear from your feedback. Wondering if FE can handle this perfectly or perhaps we can just leave it for BE. Thank you in advance!

cc @hoy @modimo @wenlei

In D130303#4175664, @collinbaker wrote:

@dexonsmith can you weigh in?

In D130303#3724392, @dexonsmith wrote:

In D130303#3724247, @rnk wrote:

Pinging alternative reviewer +@dexonsmith for a libclang API addition

Looks reasonable to me -- this only changes behaviour of the existing API when there was corruption before -- but if the goal is to get a vendor of libclang-as-a-stable-API to sign off, I can't help.

@arphaman, if you're busy, is there someone else that could take a quick look?

LGTM too.

LGTM.

In D144322#4151759, @iana wrote:

In D144322#4151757, @vsapsai wrote:

Sorry, forgot to ask the first time. This change is for Clang modules and not for C++20 modules, right? Asking because believe C++20 modules have standard-enforced module names.

Yes, as far as I know the module map is only used for Clang modules.

In D144322#4140228, @iana wrote:

In D144322#4140227, @dexonsmith wrote:

Great to see this making progress; looking forward to seeing the related workarounds removed from the Darwin SDKs.

What's the performance impact (for scanning/building/importing modules) of turning these all into top-level modules? If it's a significant regression, maybe there's a way to isolate the headers that are involved in the cycle with the SDK and the Clang headers from those that aren't, while still using only a handful of modules. And if it's a performance improvement in some cases (due to better parallelism or building less stuff), could be something to highlight.

We haven't tested the performance yet. We could try to optimize and the C headers that aren't included by the other ones could maybe stay in std. That's kind of risky though because we can't really guarantee what the include_next'ed headers will include. I think we're better off keeping the C headers in their own modules across the board (in clang and also the Apple headers).

Great to see this making progress; looking forward to seeing the related workarounds removed from the Darwin SDKs.

LGTM

It's nice how simple this is!

In D133715#4032713, @steven_wu wrote:

Maybe RawHashTrieMap? It reads better when Raw is in the front, and it contains hash-trie and trie-map, which are both terms describing data structures similar to this but this is much simpler, thus raw.

In D133715#4032581, @dblaikie wrote:

I guess one concern with TrieHashMap is that if this is the lower level implementation, and someone might implement a more map-like API on top of this, we might not want to take the "better" name for the data structure that'll be less directly used?

Could prefix with "Raw" or maybe TrieRawHashMap? (since it's the hashing part that's particularly "raw" - relying on the hash being unique, etc)

In D133713#4031859, @steven_wu wrote:

The reason I don't want to have the CAS blocked by a better allocator is that I need to write and test it, also figure out if it needs to have any code sharing with regular BumpPtrAllocator.

In D133715#4031396, @avl wrote:

But having a fast concurrent BumpPtrAllocator would be independently useful, and I'd suggest optimizing the allocator before bloating the default trie size.

+1 for fast concurrent ThreadSafeBumpPtrAllocator.

What do you think about following alternative implementation?

class ThreadSafeBumpPtrAllocator {
  ThreadSafeBumpPtrAllocator() {
    size_t ThreadsNum = ThreadPoolStrategy.compute_thread_count();
    allocators.resize(ThreadsNum);
  }
  
  void* Allocate (size_t Num) {
      size_t AllocatorIdx = getThreadIdx();
      
      return allocators[AllocatorIdx].Allocate(Num);
  }

  std::vector<BumpPtrAllocator> allocators;
};

static thread_local ThreadIdx;

size_t getThreadIdx() {
  return ThreadIdx;
}

This implementation uses the fact that ThreadPoolExecutor creates a fixed number
of threads(ThreadPoolStrategy.compute_thread_count()) and keeps them until destructed
. ThreadPoolExecutor can initialise thread local field ThreadIdx to the proper thread index.
The getThreadIdx() could return index of thread inside ThreadPoolExecutor.Threads.
ThreadSafeBumpPtrAllocator keeps separate allocator for each thread. In this case each thread would
always use separate allocator. No neccessary to have locks, cas operations, no races...

In D133715#4029879, @steven_wu wrote:

In D133715#4029841, @dexonsmith wrote:

In D133715#4029442, @steven_wu wrote:

Ping. All feedbacks are addressed.

Additional notes: I dig a bit deeper into the benchmark from https://reviews.llvm.org/D132548 as it shows bad scaling during parallel insertion tests (stop scaling to 8 threads). That is actually caused by our ThreadSafeBumpPtrAllocator that currently takes a lock. We can improve it in future since our use case doesn't expect heavy parallel insertions. However, it is quite obvious we should tune to RootBits and SubTrieBits. Increasing RootBits can significantly decrease the contention. A better strategy might be something like starting with something like 10 bits, then 4 bits, 2 bits and 1 bit. Shrinking number of bits can lead to better memory usage since the slots usage in the deep nodes are very low.

Could ThreadSafeBumpPtrAllocator could be made lock-free? I think at least it would be possible to implement one that only locked when a new allocation is needed, instead of every time the ptr is bumped as now. (I’ll think about it a bit.)

Note that in the CAS use case it’s ideally true that most insertions are duplicates and don’t need to call the allocator at all. This is why we’ve been able to get away with a lock on each allocation.

In D133715#4029442, @steven_wu wrote:

Ping. All feedbacks are addressed.

Additional notes: I dig a bit deeper into the benchmark from https://reviews.llvm.org/D132548 as it shows bad scaling during parallel insertion tests (stop scaling to 8 threads). That is actually caused by our ThreadSafeBumpPtrAllocator that currently takes a lock. We can improve it in future since our use case doesn't expect heavy parallel insertions. However, it is quite obvious we should tune to RootBits and SubTrieBits. Increasing RootBits can significantly decrease the contention. A better strategy might be something like starting with something like 10 bits, then 4 bits, 2 bits and 1 bit. Shrinking number of bits can lead to better memory usage since the slots usage in the deep nodes are very low.

LGTM with one bit inline.

In D138760#4020385, @MaskRay wrote:

In D138760#4018383, @he32 wrote:

Thanks for the hint / redirect.
I've submitted a pull request at https://github.com/BLAKE3-team/BLAKE3/pull/280

Thank you! If we take the patch here, we may risk losing the change when someone upgrades BLAKE3. So it's better to ensure that it works in the upstream...

In D132455#4018472, @dblaikie wrote:

@dexonsmith & co working on the CAS have also proposed a thread safe hash table of sorts ( https://reviews.llvm.org/D133715 )- it's a bit more esoteric/specialized, but I wonder if the use cases overlap enough to be able to unify them?

In D133715#3996103, @dblaikie wrote:

eh, maybe string's easier to read in expect diagnostics anyway... just seems a bit awkward/circuitous/unfortunate :/ (I guess the stringification could move into the test code, implemented via such a friend relationship)

In D133715#3995750, @dblaikie wrote:

In D133715#3993521, @dexonsmith wrote:

In D133715#3993489, @dblaikie wrote:

• a test support friend class that can inspect/test the trie layout without relying on stringification;

This bit I'm not so sure about - we don't test the bucketing behavior of DenseHashMap so far as I know (or other similar implementation details of the various other hashing data structures - (since they're the ones with fairly complicated implementation details, they seem the best comparison)), for instance, we only test its interface - why would we do differently for this data structure?

IMO the layout is more complex than a flat array with quadratic probing. The logic for equal and/or nearly-equal hashes is a bit subtle, and there were hard-to-reason about bugs originally. The layout-stringification was necessary to understand what was going wrong, and the tests that use it help ensure future refactorings don't get it wrong.

I don't remember the bugs, but two examples of subtleties:

• On an exact match you don't want to "sink" the existing entry down a level to a new sub-trie (you need to detect "exact match" before sinking). Getting this wrong will affect performance but not otherwise be user-visible.
• The deepest sub-trie might be a different/smaller size than the others because there are only a few bits left-over, and the handling needs to be right. It's simpler to check for correct layout directly than to guess about what user-visible effects there might be for errors.

I'd be a bit uneasy with the layout tests being dropped altogether.

Maybe an alternative to testing the layout directly would be to add a verification member function that iterated through the data structure and ensured everything was self-consistent (else crash? else return false?). Then the tests could call the member function after a series of insertions that might trigger a "bad" layout.

Fair enough - if it's sufficient to have a verify operation (maybe "assertValid" - so, yeah, crash when not valid) I'd go with that, but given the argument you've made, if you think verifying the specific structure is significantly more valuable than that, I'd be OK with some private/test-friended introspection API.

In D133715#3993489, @dblaikie wrote:

• a test support friend class that can inspect/test the trie layout without relying on stringification;

This bit I'm not so sure about - we don't test the bucketing behavior of DenseHashMap so far as I know (or other similar implementation details of the various other hashing data structures - (since they're the ones with fairly complicated implementation details, they seem the best comparison)), for instance, we only test its interface - why would we do differently for this data structure?

In D133715#3993311, @dblaikie wrote:

Note that there's an example data structure called ThreadSafeHashMappedTrieSet buried in the unit tests, which has an interface that hides the hash. Could be useful for some clients.

• Maybe it would make sense to lift up? Name could be PerfectHashSet
• You might want to build a map that also handles the hashing; name could potentially be PerfectHashMap

Maybe this could be PerfectHashSetImpl?

• Has the "guts" for implementing a client-friendly PerfectHashSet or PerfectHashMap?
• ... but can be used directly if you want to manage the hashing yourself?

I take it the current use cases you have in mind/lined up in the CAS use this directly? Maybe a Raw prefix? RawPerfectHashSet, RawConcurrentHashSet, etc... some combination/choice of those sort of things?

In D133715#3993178, @dblaikie wrote:

Is it a set or a map?

In D139163#3991248, @MoritzS wrote:

To answer your questions in the comments about what to do about resolvers with arguments: At least glibc always calls ifunc resolvers without any arguments. It just reads the address of the resolver function from the ELF file, casts it to a pointer to a void-returning function with no arguments and calls it.

So, I agree: Resolvers with arguments should not be allowed.

In D139767#3986728, @DarshanRamakant wrote:

In D139767#3986476, @dexonsmith wrote:

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

Thanks @dexonsmith for the suggestion. But I think even if we want to downcast ( dynamic_cast) the declaration should be available to the derived class, which is not the case in the current implementation.

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

[Resigning as reviewer, since I was the original author, but feel free ping me explicitly if I can be helpful for something and I'm not volunteering anything...]

In D138796#3975547, @ahatanak wrote:

In D138796#3967899, @dexonsmith wrote:

In D138796#3957499, @ahatanak wrote:

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

Great progress; seems to me like the regression is old enough it might not be an easy fix. Are you okay with the tests being cleaned up to reflect reality in the meantime?

Yes, I think that's okay. We are tracking this internally, so this can be committed as-is.

In D138796#3957499, @ahatanak wrote:

I confirmed ARC optimizer hasn't removed the retain/release pairs in both functions at least since October 2017. The retain/release pair in @"\01-[Top0 _getX]" can be removed. I'm not sure about the one in @"\01-[A z]".

LGTM.

In D133716#3958319, @steven_wu wrote:

Ping.

Now with context of the LLVM talk, I hope it makes the review easier. Will be really appreciated for any feedback to get CAS upstream. Feel free to reach out to me for any questions.

Here's a link to a related RFC:
https://discourse.llvm.org/t/rfc-constantdata-should-not-have-use-lists/42606

Separate flags allows vendors (and the default!) to have use-list order on-by-default in bitcode and off-by-default in textual IR.

@ahatanak or @rjmccall, can you confirm whether a bug has crept in while the test was broken? Is a retain/release around @printf and/or @puts expected? Or maybe there's no bug, but the printf/puts declarations need to be annotated somehow?

I just realized @jansvoboda11 is probably out on holiday this week (IIRC, Apple usually gets this week off). Since this was committed almost a month ago, I'm guessing this isn't enough of a blocker that we need to revert rather than wait until next week (and there are some commits on top that rely on this!). But probably reverting the stack would be an option if it's critical.

LGTM!

In D137164#3917926, @MichielDerhaeg wrote:

In D137164#3915823, @dexonsmith wrote:

Would it be simpler to define < for that case? Or, we could define a custom one to send to the std::map:

struct CompareValID {
  static bool operator<(const ValID &LHS, const ValID &RHS) {
    if (LHS.Kind != RHS.Kind)
      return LHS.Kind < RHS.Kind;
    return LHS < RHS;
  }
};

That was my initial idea, but if you search for other ForwardRef-related datastructures in LLParser.cpp, you'll see that they solved it this way for all other cases as well.
I did it like this to remain consistent.

Would it be simpler to define < for that case? Or, we could define a custom one to send to the std::map:

struct CompareValID {
  static bool operator<(const ValID &LHS, const ValID &RHS) {
    if (LHS.Kind != RHS.Kind)
      return LHS.Kind < RHS.Kind;
    return LHS < RHS;
  }
};

In D137473#3915497, @bnbarham wrote:

This seems reasonable to me in general. @dexonsmith in case you have any thoughts.

Consider making the FileEntryRef changes here the default -- it doesn't make sense to me that FileEntryRef == or DenseMap would match FileEntry pointer semantics instead of filename semantics.

LGTM, with one suggestion for the test inline.

LGTM.

LGTM, although I there's format-is-probably-compatible-version as a courtesy for tooling, does that need a bump here?

LGTM.

LGTM.

LGTM.

In D136624#3900593, @jansvoboda11 wrote:

Ah, I forgot to mention this. Building the modules is now only 0.2% slower and importing them 1.2% faster (compared to PCMs with all input files serialized).

In D136624#3900051, @jansvoboda11 wrote:

• Is there a change in cycles/instructions when the module cache is hot? (presumably the common case)

I didn't notice this (but didn't look for it specifically). How could that affect performance for PCM writes?

In D135220#3898849, @hans wrote:

Relatedly, we ran into a problem with clang-cl /showIncludes not including all files in its output when they're linked: https://github.com/llvm/llvm-project/issues/58726

In D135220#3870991, @hans wrote:

One thought, which I'm not sure is relevant, is that this is only observable for headers which are included more than once, which is rare because normally there are include guards (or pragma once). isFileMultipleIncludeGuarded isn't tracked by the FileManager, but otherwise maybe that could have been a useful heuristic: don't merge header files without include guards?

In D136624#3893001, @jansvoboda11 wrote:

I tried implementing your suggestion (merging ranges of adjacent non-affecting files and avoiding FileID lookups), but the numbers from -ftime-trace are very noisy. I got more stable data by measuring clock cycles and instruction counts, but nothing conclusive yet.

Compilation of CompilerInvocation.cpp with implicit modules.

• previous approach with vector + FileID lookup: +0.64% cycles and +1.68% instructions,
• current approach with merged SourceRanges: +0.38% cycles and +1.11% instructions.

I'll post here as I experiment more and get more data.

In D136624#3888387, @jansvoboda11 wrote:

I tried optimizing this patch a bit. Instead of creating compact data structure and using binary search to find the preceding non-affecting file, I now store the adjustment information for each FileID in a vector. During deserialization, FileID is simply used as an index into SLocEntryInfos. That didn't yield any measurable improvement in performance, though. I think the regression must be coming from the SourceLocation/Offset to FileID translation.

I don't see any obvious way to work around that. SourceManager::getFileIDLocal() already implements some optimizations that makes accessing nearby offsets fast. A separate SourceManager would avoid this bottleneck, but I'm not sure how much work that would entail (seems substantial).

@Bigcheese LMK if you're fine with the performance implications here.

In D106876#3869447, @jansvoboda11 wrote:

I agree that avoiding serializing non-affecting input files is the better approach. Your code is more or less what I had in mind, thanks for sketching it out!
The number of ignored module maps is typically around 70 - 110 on macOS (when you allow system module maps to be treated as non-affecting), and those are at the start of the SourceManager block. I might implement this approach and test out if there's a noticeable performance impact. Maybe we could use something similar to the optimization in SourceManager::getFileIDLoaded().

In D106876#3869247, @dexonsmith wrote:

A further (more involved) approach would be to separate module maps into a separate SourceManager, so that their source locations don't affect other input files. Then only module map source locations would need to be translated during serialization. (Now that FileManager has the capability to remap file contents, I think the commit that merged the SourceManagers could be effectively reverted.)

As an end goal, it seems strictly better for build systems / distribution / artifact storage / caching if the output PCM has been canonicalized as-if the module maps weren't found, rather than tracking both which module maps were discovered and which to ignore. Ideally, we'd find a way to canonicalize the output efficiently and correctly... along the lines of this patch, but with a bug fix somewhere.

In D135220#3862058, @hans wrote:

The build system folks replied saying they're not using symlinks, but hard links for compiler inputs. Dropping the -s flag in the repro above demonstrates this.

I would think we could convert every assert(0) to either llvm::report_fatal_error (guaranteed trap) or llvm_unreachable() (trap or optimize, depending on CMAKE configuration). The C API usage checks seem like good candidates for the former.

In D135220#3840257, @goncharov wrote:

It's possbile to make it deterministic by making headers unique though.

In D135220#3840257, @goncharov wrote:

In D135220#3840177, @dexonsmith wrote:

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve consistency for symlinked content. Knowing the failure mode / having reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to make forward progress.)

What I see in this clang/test/Driver/cl-pch-showincludes.cpp is that e.g. for run on :25 it now emits

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header1.h

instead of

Note: including file: ...header2.h
Note: including file: ...header1.h
Note: including file: ...header3.h

as header3 -> header1. It's possbile to make it deterministic by making headers unique though.

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

SGTM.

In D134456#3827478, @aaron.ballman wrote:

In D134456#3827410, @dexonsmith wrote:

In D134456#3827377, @aaron.ballman wrote:

Another thought I had is: when PGO is enabled, can we diagnose when PGO countermands an explicit annotation? Something to at least alert the user "hey, look over here, this suggestion was wrong" so they have more of a chance of knowing something is up?

Seems pretty noisy, much like when the inliner countermands an inline hint. Maybe an optimization remark would be appropriate, allowing advanced users to selectively enable them in a critical section when debugging a performance issue?

Hmmm, on the one hand, any optimization hint is really an advanced user feature and so the opt remark might make sense. On the other hand, when I posted about [[likely]] and PGO on Twitter (not exactly what I'd call a scientific poll, take with an ocean of salt!), multiple people were surprised and expected PGO to honor the attribute, which suggests users may want this information. I don't have a good feel for what the user expectations are here, so I think an opt remark would be the most conservative way to start, and if we find that it's too hidden and non-expert users really need warnings, we can add a warning at that point. WDYT?

In D134456#3827377, @aaron.ballman wrote:

In D134456#3827332, @dexonsmith wrote:

In D134456#3827263, @aaron.ballman wrote:

My worry is that parallel attributes will be used as:

#if __has_cpp_attribute(clang::likely_but_honor_this_one)
#define LIKELY [[clang::likely_but_honor_this_one]]
#elif __has_cpp_attribute(likely)
#define LIKELY [[likely]]
#else
#define LIKELY
#endif

To be clear, I was imagining:

if (always_false()) [[likely]] [[clang::nopgo]] {
  // ...
}

where `nopgo` might be independently useful for telling clang to ignore any collected PGO data when estimating branch weights in a particular control flow block.

Some users might combine the two into a macro ("always ignore the profile when I say something is likely!"), but I don't think there'd be a cascade.

Ah, I see, thank you for clarifying! Does that seem like a generally useful attribute to you? (It seems like it could be, but this isn't my area of expertise.)

In D134456#3827332, @dexonsmith wrote:

In D134456#3827263, @aaron.ballman wrote:

My worry is that parallel attributes will be used as:

#if __has_cpp_attribute(clang::likely_but_honor_this_one)
#define LIKELY [[clang::likely_but_honor_this_one]]
#elif __has_cpp_attribute(likely)
#define LIKELY [[likely]]
#else
#define LIKELY
#endif

To be clear, I was imagining:

if (always_false()) [[likely]] [[clang::nopgo]] {
  // ...
}

where `nopgo` might be independently useful for telling clang to ignore any collected PGO data when estimating branch weights in a particular control flow block.

Some users might combine the two into a macro ("always ignore the profile when I say something is likely!"), but I don't think there'd be a cascade.