Good catch, thanks for the fix.

Updating D130717: [SCCIterator] Fix an issue in scc_member_iterator sorting

In D83906#4187872, @dexonsmith wrote:

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 sure I'm following.

The prelink optimizer will already be internalizing (i.e., NOT exporting) these symbols. That should be enough. AFAICT, it's non-LTO pipelines that might have headroom after this is reverted.

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

In D83906#4182904, @dexonsmith wrote:

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?

Depends what you mean by "trusted". It assumes the attributes accurately describe the function it sees. The properties promised there will apply if/when the code is inlined. But, since the commit in 2016, it doesn't trust that they fully describe the source semantics, so IPA ignores them when the function is not inlined.

Note that the optimizer doesn't know if its input IR has already been optimized. Is this the first optimizer that has run on the IR, or could side effects have been refined away already? E.g., if the optimization pipeline in question runs at LTO time, the compile-time optimization pipeline has already run.

In D83906#4182476, @dexonsmith wrote:

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.

The frontend has generated unrefined IR with all side effects from the must-be-ODR-equivalent source still present. It's not until on optimizer gets at it that side effects can be refined away. (Unless the IRGen peepholes are powerful enough to refine away side effects, but I don't believe IRGen does that.)

Since the IR from IRGen is unrefined (still has all side effects present in the source), whatever the linker/loader chooses cannot gain "extra" side effects through de-refinement.

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.

In D145171#4182014, @xur wrote:

The total improvement really depends on programs. On average, I would say 1.5% to 2.0%. For some programs, like clang itself, FSAFDO improve ~3% over AFDO.
There is also a small change on the create_llvm_prof tool side. We will update the tool soon.

In D83906#4180435, @dexonsmith wrote:

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

Since 2016, the optimizer is not allowed to do IPA on functions that can be de-refined (such as linkonce_odr functions).

Here's a hypothetical problematic scenario for the optimizer:

• original IR for B has a throw somewhere
• function A invokes function B
• in this TU, B is optimized and removes exceptions, and gets marked nounwind
• function A leverages the nounwind to turn the invoke into a call
• function B is de-refined at link/load time: linker chooses a *different* function B which still has a throw
• "evil magic" happens (see the discussions around the patch linked above)
• a crash is introduced

At first blush, it sounds like this could only be a problem if the code has UB in it. However, read the above patch (and follow-ups, and related discussion) for a variety of examples of non-UB cases where IPA on de-refineable functions introduces crashes. I don't know for sure this could be a problem for nounwind specifically, but in general the LLVM optimizer doesn't look at attributes of de-refineable functions.

(Note that if you're doing LTO (like AutoFDO), this usually won't block optimization, since at LTO time there are very few de-refineable functions (most linkonce_odr functions are internalized, and not exported as weak). So if we added a -cc1 flag to prefer "stable IR" over "frontend peepholes", it would make sense for -flto to imply it.)

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.

---

BTW, I'm not personally against removing this peephole from the frontend (even without a flag), or limiting it somehow to cases where it doesn't make IR output unstable. I like the idea of stable IRGen output.

Nevertheless, it feels like removing IPA-based peepholes from Clang in the name of stable IRGen output is a change in direction, which might deserve a discussion in the forums rather than in a particular patch review.

(Also, can the backend safely optimize an invoke to a linkonce_odr function that's nounwind to a call? I thought it couldn't, in case the function is de-refined to a version that's not nounwind. But the frontend can do it since it has access to the source and knows it can't be de-refined in that way?)

In D145171#4179269, @xur wrote:

The performance for removing BBSize from discriminator hash is out: it shows a slightly gain on performance, like ~0.2% vs with BBsize in the hash. I will remove BBSize from the hash.

Thanks for the change! I'm going to start the integration with pseudo probe from here.

Updating D144066: [Pseudo probe] Duplicate probes in vectorized loop body.

LGTM, thanks for fixing it.

Removing unnecessary header inclusions.

moving the predecessors/successors definition from MachineSSAContext.h to MachineBasicBlocks.h

Updating D142747: [Pseudo Probe] Do not instrument EH blocks.

Updating D142747: [Pseudo Probe] Do not instrument EH blocks.

Addressing feedbacks.

In D142747#4087112, @modimo wrote:

In D142747#4087088, @modimo wrote:

In D142747#4087069, @wenlei wrote:

In D142747#4086978, @modimo wrote:

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

That diff also used EHPad as seeds for BFS, I thought we only need LandingPads as seeds? Though I think for simple algorithm, having MIR and IR implementation separated is probably fine. I also don't know where to put a merged version..

if (MBB.isEHPad())
  LandingPads.push_back(&MBB);

LandingPads are the only thing that exists for x86_64 Linux while catchswitch, catchpad, and cleanuppad are all for Windows EH (https://llvm.org/docs/ExceptionHandling.html#new-exception-handling-instructions). Without proper testing on Windows I'd advocate for keeping it to LandingPads only.

For where to put it, right now I'd say there's not a good place for it and we may want to create a new file. Clang has llvm-project/clang/lib/CodeGen/CGException.cpp for all of it's EH support structure but EH on LLVM is very haphazard (I suspect because outside of correctness there was not much optimization thought put into it).

I would support adding a llvm/lib/Analysis/Exceptions.cpp (or whatever name makes sense) like llvm/lib/Analysis/EHPersonalities.cpp exists. Otherwise, if we don't want to add another file I would place it in Function.h/cpp since technically we can consider this calculating a property of the function.

In D142747#4087088, @modimo wrote:

In D142747#4087069, @wenlei wrote:

In D142747#4086978, @modimo wrote:

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

That diff also used EHPad as seeds for BFS, I thought we only need LandingPads as seeds? Though I think for simple algorithm, having MIR and IR implementation separated is probably fine. I also don't know where to put a merged version..

if (MBB.isEHPad())
  LandingPads.push_back(&MBB);

LandingPads are the only thing that exists for x86_64 Linux while catchswitch, catchpad, and cleanuppad are all for Windows EH (https://llvm.org/docs/ExceptionHandling.html#new-exception-handling-instructions). Without proper testing on Windows I'd advocate for keeping it to LandingPads only.

Updating D142747: [Pseudo Probe] Do not instrument EH blocks.

In D142747#4086978, @modimo wrote:

We've got the same EH-ness propagation code in MFS already: https://reviews.llvm.org/D131824. Consider merging the code?

In D142747#4086745, @wlei wrote:

Will this affect PROFI? if we don't insert probe for EH block, does PROFI assign a cold(0) value to the BB or leave it with an unknown value like the dangling probe?

Updating D142747: [Pseudo Probe] Do not instrument EH blocks.

LGTM.

Switching to using single maps.

In D140063#3998381, @wlei wrote:

In D140063#3998253, @wenlei wrote:

I thought the idea is to compute the % of samples being dropped due to mismatch? in this case, all samples from callsite will be dropped, so I actually don't see a problem with using getTotalSamples. Yes you can argue that they can be merged with base profile, but then that argument applies to getHeadSamplesEstimate too?

Yeah, it's tricky to quantify the dropped callsite samples's impact. I was thinking in a reversed way, if we use getTotalSamples, I feel like the total samples are completely dropped(not merged into top-level profile), in fact it's still in use. And for getHeadSamplesEstimate, I feel it's like only the callsite call/jump's samples are dropped, but I admit that this's also not accurate, the missing inlining from big total samples could affect more on perf. I don't have strong opinion on this.

Updating D139367: [CSSPGO][llvm-profgen] Missing frame inference.

Bounding the DFS search with a speific size (default to INT32_MAX). Will run more experiments to settle down on a good limit.

Addressing feedbacks.

Addressing feedbacks.

Addressing feedbacks.

lgtm, thanks.

In D138231#3934734, @wlei wrote:

I think we actually will use the ProfileInlineLimitMin which is 100 for the count, see SizeLimit = std::max(SizeLimit, (unsigned)ProfileInlineLimitMin);

ProfileInlineGrowthLimit is by default set to 12, and 1 * 12 is still smaller than 100.