Updating D104477: [CSSPGO] Undoing the concept of dangling pseudo probe

Addressing Wenlei's comment.

Addressing Wenlei and Lei's comments.

Fixing clang-tidy issue.

In D104129#2821245, @wlei wrote:

Curious why we can remove the moveAndDanglePseudoProbes(don't need the concept of dangling)? My understanding is we treat dangling as unknown, at pass1 we need to pass this information to the profiled binary. An optimized-out probe won't have samples, it will be treated as zero if not marked as dangling.

In D104129#2821201, @wenlei wrote:

In D104129#2821189, @hoy wrote:

In D104129#2821164, @wenlei wrote:

Be consistent with dwarf line number based profile.

This is not accurate, see earlier comments. What i meant was not about "non-probe profile -> dwarf line number profile".

Otherwise lgtm. Thanks.

It should be consistent with non-CS dwarf-based profile. CS dwarf-based profile doesn't have either zero or dangling reported, likely needs a fix. Is that what you meant?

Non-cs dwarf-based profile normally doesn't have zero counts in the profile. Any count below a certain threshold is omitted in the profile to save size. This is the default behavior for our production usage (I believe for google as well). This is why the current change that needs explicit zero is not consistent with afdo.

In D104129#2821164, @wenlei wrote:

Be consistent with dwarf line number based profile.

This is not accurate, see earlier comments. What i meant was not about "non-probe profile -> dwarf line number profile".

Otherwise lgtm. Thanks.

Updating summary and comments.

In D103988#2820746, @xur wrote:

In D103988#2820735, @MaskRay wrote:

CommonLinkage doesn't work.

Whether appendToUsed is needed depends on whether the runtime has a live section referencing __llvm_fs_discriminator__.
If the runtime has the guarantee, no need for appendToUsed.

Otherwise, the first revision appendToUsed with WeakODRLinkage looks good to me.

appendToUsed with ExternalLinkage in a comdat works as well, but comdat will need extra code when Mach-O wants this feature.

Fangri, thanks for reviewing this. This variable will NOT be referenced by compiler runtime library -- only be referenced by AutoFDO offline tool (to generate AFDO profile).

It seems that Fangri (as a linker expert) prefers my first implementation.

I'm OK either way. Hoy: what is your thought?

Removing InvalidProbeCount definition and uses.

Addressing Wenlei's comment.

In D104129#2818629, @wenlei wrote:

Thanks for working on this. Among all deleted probes, if the majority of them are actually dangling (i.e removed due to hoisting, merging like optimization as opposed to dce), it makes sense to default to dangling and explicitly marking non-dangling. Previously we assumed that majority of deleted probes are not dangled, hence we choose to mark dangled ones explicitly.

It seems that our assumption has been wrong, which is why this change actually improves profile quality.

This enabled dangling probe to be represented by both INT64_MAX and none (missing entry in profile).

Why do we still want to allow INT64_MAX representation of dangling probe? Would it be cleaner to only use missing probe to represent dangled probes?

Updating D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes.

In D104129#2817282, @spupyrev wrote:

It is strange that representation of probes affects counts quality (and hence, generated binaries). Is this a temporary state (that hopefully will be improved in the future)?

In general I agree that the suggested representation (0 for zero-counts, missing for dangles) is way more readable, and thus, less-error-prone.

LGTM.

Nit: adjust the summary for the part of testing and the disabling logic.

In D103867#2806612, @aeubanks wrote:

do all passes that clone BBs have to worry about this?

Will come back with more numbers.

LGTM. Please wait to see if @davidxl and @wenlei has additional comments.

LGTM, thanks for working on this.

Addressing Wenlei's comment.

Addressing Wenlei's feebacks.

As discussed offline, we may want to experiment treating all missing probes as dangling in the final pass of the pipeline and see how effective the counts inference can be. This would avoid such ad-hoc fixes including the previous ones we made.

Thanks for the fix. Is it possible to add a test?

Addressing Wei's comment.

In D102537#2763883, @wenlei wrote:

In D102537#2762431, @hoy wrote:

In D102537#2761666, @wenlei wrote:

Thanks for bringing this up - it makes sure we give full context profile precedence even when we had to use context-less profile in prelink.

When counts are accurate, I was seeing #1 and #2 help reduce code size by preventing post-sample ICP and CGSCC inliner working on obselete metedata.

I assume the above is from CSSPGO, right?

It'd be good to call out in the change description as to why postlink overwrite could be beneficial.

Yes, they are for CSSPGO.

If we target AutoFDO as well, would be good to mention the impact on AutoFDO too.

I haven't tested for non-CS case. In theory it should help there but with a less-than-ideal counts quality, the change may not help in practice.

For future reference, can you make it explicit in the change description: 1) what made this beneficial for CSSPGO (give post-link full context profile precedence over pre-link partial context profile, i.e. be more specific about obsolete metadata); 2) what is the expected impact on AutoFDO.

I think there's another caveat worth calling out, by overwriting all pre-link weights, we would lose the prof metadata adjustment done by prelink opt passes (e.g. prelink loop passes).

Addressing Wenlei's comment.

In D102537#2761666, @wenlei wrote:

Thanks for bringing this up - it makes sure we give full context profile precedence even when we had to use context-less profile in prelink.

When counts are accurate, I was seeing #1 and #2 help reduce code size by preventing post-sample ICP and CGSCC inliner working on obselete metedata.

I assume the above is from CSSPGO, right?

It'd be good to call out in the change description as to why postlink overwrite could be beneficial.

Addressing Wenle's comments.

Addressing Wei's comment.

In D102356#2760973, @dblaikie wrote:

In D102356#2758371, @hoy wrote:

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

In D102429#2760347, @wlei wrote:

Try to understand this:

So we merge the samples from the same function but different context, does this cause the issue?

In D102356#2758179, @dblaikie wrote:

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

In D98799#2757959, @dblaikie wrote:

I took another look. I think the divergence comes from getAs<FunctionProtoType> vs hasPrototype. The debug data generation uses hasPrototype while getAs<FunctionProtoType> is used as overloadable attribute processing as long as unique linkage name processing before this change. More specifically, the following function definition is represented by FunctionProtoType while it does not hasPrototype.

Ah, sorry, maybe I'm coming around to this - so you're saying that the test in ItaniumMangleContextImpl::isUniqueInternalLinkageDecl must match the check in CGDebugInfo::collectFunctionDeclProps And when they diverge something bad happens? (could you refresh me on what that breaks - something crashes in ObjectiveC test cases? Or the tests fail?)

I wonder whether we should change both of them then?