Page MenuHomePhabricator

hoy (Hongtao Yu)
Software Engineer at Facebook

Projects

User does not belong to any projects.

User Details

User Since
Feb 23 2020, 3:46 PM (68 w, 4 d)

Recent Activity

Today

hoy added reviewers for D104477: [CSSPGO] Undoing the concept of dangling pseudo probe: wenlei, wlei, wmi.
Thu, Jun 17, 11:27 AM · Restricted Project
hoy updated the diff for D104477: [CSSPGO] Undoing the concept of dangling pseudo probe.

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

Thu, Jun 17, 11:27 AM · Restricted Project
hoy requested review of D104477: [CSSPGO] Undoing the concept of dangling pseudo probe.
Thu, Jun 17, 11:21 AM · Restricted Project
hoy updated the diff for D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..

Addressing Wenlei's comment.

Thu, Jun 17, 9:13 AM · Restricted Project

Yesterday

hoy updated the diff for D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..

Addressing Wenlei and Lei's comments.

Wed, Jun 16, 1:08 PM · Restricted Project
hoy added inline comments to D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..
Wed, Jun 16, 12:42 PM · Restricted Project
hoy added inline comments to D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..
Wed, Jun 16, 12:12 PM · Restricted Project
hoy added inline comments to D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..
Wed, Jun 16, 12:00 PM · Restricted Project
hoy committed rGcef9b96b01b7: [CSSPGO] Report zero-count probe in profile instead of dangling probes. (authored by hoy).
[CSSPGO] Report zero-count probe in profile instead of dangling probes.
Wed, Jun 16, 11:46 AM
hoy closed D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Wed, Jun 16, 11:46 AM · Restricted Project
hoy updated the summary of D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Wed, Jun 16, 11:29 AM · Restricted Project
hoy updated the diff for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

Fixing clang-tidy issue.

Wed, Jun 16, 10:05 AM · Restricted Project
hoy added a comment to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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.

Wed, Jun 16, 9:20 AM · Restricted Project

Tue, Jun 15

hoy updated the summary of D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Tue, Jun 15, 11:51 PM · Restricted Project
hoy added a comment to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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.

Tue, Jun 15, 11:38 PM · Restricted Project
hoy added a comment to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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.

Tue, Jun 15, 11:20 PM · Restricted Project
hoy updated the diff for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

Updating summary and comments.

Tue, Jun 15, 10:55 PM · Restricted Project
hoy updated the summary of D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Tue, Jun 15, 10:54 PM · Restricted Project
hoy added inline comments to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Tue, Jun 15, 9:53 PM · Restricted Project
hoy added a comment to D103988: SampleFDO] place the discriminator flag variable into the used list..

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?

Tue, Jun 15, 5:01 PM · Restricted Project
hoy added inline comments to D103988: SampleFDO] place the discriminator flag variable into the used list..
Tue, Jun 15, 4:59 PM · Restricted Project
hoy updated the diff for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

Removing InvalidProbeCount definition and uses.

Tue, Jun 15, 3:53 PM · Restricted Project
hoy added inline comments to D104276: [CSSPGO][llvm-profgen] Ignore LBR records after interrup transition.
Tue, Jun 15, 12:32 PM · Restricted Project
hoy updated the diff for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

Addressing Wenlei's comment.

Tue, Jun 15, 9:59 AM · Restricted Project
hoy added a comment to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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?

Tue, Jun 15, 9:32 AM · Restricted Project

Mon, Jun 14

hoy added reviewers for D104276: [CSSPGO][llvm-profgen] Ignore LBR records after interrup transition: wenlei, wlei, wmi.
Mon, Jun 14, 6:35 PM · Restricted Project
hoy requested review of D104276: [CSSPGO][llvm-profgen] Ignore LBR records after interrup transition.
Mon, Jun 14, 6:34 PM · Restricted Project
hoy added reviewers for D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner.: wenlei, wlei, wmi.
Mon, Jun 14, 2:52 PM · Restricted Project
hoy requested review of D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner..
Mon, Jun 14, 2:51 PM · Restricted Project
hoy updated the diff for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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

Mon, Jun 14, 1:04 PM · Restricted Project
hoy added a comment to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

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.

Mon, Jun 14, 12:59 PM · Restricted Project

Sat, Jun 12

hoy accepted D104131: [CSSPGO] Aggregation by the last K context frames for cold profiles.

LGTM.

Sat, Jun 12, 11:04 AM · Restricted Project

Fri, Jun 11

hoy accepted D103988: SampleFDO] place the discriminator flag variable into the used list..
Fri, Jun 11, 2:27 PM · Restricted Project
hoy added a comment to D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

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

Fri, Jun 11, 12:19 PM · Restricted Project
hoy accepted D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

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

Fri, Jun 11, 12:17 PM · Restricted Project
hoy planned changes to D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..

Will come back with more numbers.

Fri, Jun 11, 10:18 AM · Restricted Project
hoy added reviewers for D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes.: wenlei, wlei, wmi.
Fri, Jun 11, 9:44 AM · Restricted Project
hoy requested review of D104129: [CSSPGO] Report zero-count probe in profile instead of dangling probes..
Fri, Jun 11, 9:42 AM · Restricted Project

Thu, Jun 10

hoy added inline comments to D103988: SampleFDO] place the discriminator flag variable into the used list..
Thu, Jun 10, 10:29 AM · Restricted Project
hoy accepted D103289: A post-processing for BFI inference.

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

Thu, Jun 10, 9:47 AM · Restricted Project

Wed, Jun 9

hoy committed rG64b2fb7967a7: [CSSPGO] Emit mangled dwarf names for line tables debug option under -fpseudo… (authored by hoy).
[CSSPGO] Emit mangled dwarf names for line tables debug option under -fpseudo…
Wed, Jun 9, 10:46 AM
hoy closed D103909: [CSSPGO] Emit mangled dwarf names for line tables debug option under -fpseudo-probe-for-profiling.
Wed, Jun 9, 10:46 AM · Restricted Project

Tue, Jun 8

hoy added reviewers for D103909: [CSSPGO] Emit mangled dwarf names for line tables debug option under -fpseudo-probe-for-profiling: wenlei, dblaikie.
Tue, Jun 8, 9:37 AM · Restricted Project
hoy requested review of D103909: [CSSPGO] Emit mangled dwarf names for line tables debug option under -fpseudo-probe-for-profiling.
Tue, Jun 8, 9:35 AM · Restricted Project

Mon, Jun 7

hoy added inline comments to D103289: A post-processing for BFI inference.
Mon, Jun 7, 9:13 AM · Restricted Project

Fri, Jun 4

hoy accepted D103640: Add an option to hide "cold" blocks from CFG graph.

LGTM, thanks for working on this.

Fri, Jun 4, 6:23 PM · Restricted Project
hoy accepted D103550: [SampleFDO] New hierarchical discriminator for FS SampleFDO (llvm-profdata part).
Fri, Jun 4, 10:35 AM · Restricted Project

Thu, Jun 3

hoy added inline comments to D103550: [SampleFDO] New hierarchical discriminator for FS SampleFDO (llvm-profdata part).
Thu, Jun 3, 5:26 PM · Restricted Project
hoy accepted D103650: [CSSPGO][llvm-profgen] Make extended binary the default output format.
Thu, Jun 3, 3:17 PM · Restricted Project

Tue, Jun 1

hoy added inline comments to D103178: [CSSPGO][llvm-profgen] Allow multiple executable load segments..
Tue, Jun 1, 12:58 PM · Restricted Project

Thu, May 27

hoy added reviewers for D103289: A post-processing for BFI inference: wmi, davidxl.
Thu, May 27, 3:46 PM · Restricted Project

Wed, May 26

hoy accepted D103041: [SampleFDO] New hierarchical discriminator for FS SampleFDO (ProfileData part).
Wed, May 26, 9:40 PM · Restricted Project
hoy updated the summary of D103178: [CSSPGO][llvm-profgen] Allow multiple executable load segments..
Wed, May 26, 9:11 AM · Restricted Project
hoy requested review of D103178: [CSSPGO][llvm-profgen] Allow multiple executable load segments..
Wed, May 26, 9:09 AM · Restricted Project

Tue, May 25

hoy added inline comments to D103041: [SampleFDO] New hierarchical discriminator for FS SampleFDO (ProfileData part).
Tue, May 25, 11:20 PM · Restricted Project
hoy accepted D103071: [CSSPGO][llvm-profgen] Change default cold threshold for context merging.
Tue, May 25, 9:47 AM · Restricted Project

Mon, May 24

hoy committed rG00bfde723b64: [NFC][CSSPGO]llvm-profge] Fix Build warning dueo to an attrbute usage. (authored by hoy).
[NFC][CSSPGO]llvm-profge] Fix Build warning dueo to an attrbute usage.
Mon, May 24, 12:59 PM
hoy committed rG3b51b51877ee: [CSSPGO][llvm-profgen] Report samples for untrackable frames. (authored by hoy).
[CSSPGO][llvm-profgen] Report samples for untrackable frames.
Mon, May 24, 12:39 PM
hoy closed D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Mon, May 24, 12:39 PM · Restricted Project
hoy added inline comments to D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Mon, May 24, 12:28 PM · Restricted Project
hoy updated the diff for D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..

Addressing Wenlei's comment.

Mon, May 24, 12:28 PM · Restricted Project
hoy updated the diff for D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..

Addressing Wenlei's feebacks.

Mon, May 24, 12:19 PM · Restricted Project
hoy added inline comments to D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Mon, May 24, 12:12 PM · Restricted Project
hoy added inline comments to D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Mon, May 24, 10:37 AM · Restricted Project

Fri, May 21

hoy updated the summary of D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Fri, May 21, 6:14 PM · Restricted Project
hoy requested review of D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames..
Fri, May 21, 6:10 PM · Restricted Project

Wed, May 19

hoy added inline comments to D102808: [NFC][SampleFDO] Add more debugging logic..
Wed, May 19, 2:42 PM · Restricted Project
hoy added reviewers for D102808: [NFC][SampleFDO] Add more debugging logic.: wenlei, wmi.
Wed, May 19, 1:57 PM · Restricted Project
hoy requested review of D102808: [NFC][SampleFDO] Add more debugging logic..
Wed, May 19, 1:56 PM · Restricted Project
hoy committed rG4ca6e37b9825: [CSSPGO] Overwrite branch weight annotated in previous pass. (authored by hoy).
[CSSPGO] Overwrite branch weight annotated in previous pass.
Wed, May 19, 9:26 AM
hoy closed D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..
Wed, May 19, 9:26 AM · Restricted Project
hoy accepted D102721: [CSSPGO] Avoid deleting probe instruction in FoldValueComparisonIntoPredecessors.

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.

Wed, May 19, 9:23 AM · Restricted Project

May 18 2021

hoy added a comment to D102721: [CSSPGO] Avoid deleting probe instruction in FoldValueComparisonIntoPredecessors.

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

May 18 2021, 2:43 PM · Restricted Project
hoy retitled D102537: [CSSPGO] Overwrite branch weight annotated in previous pass. from [SampleFDO] Overwrite branch weight annotated in previous pass. to [CSSPGO] Overwrite branch weight annotated in previous pass..
May 18 2021, 12:25 PM · Restricted Project
hoy updated the diff for D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..

Addressing Wei's comment.

May 18 2021, 12:25 PM · Restricted Project
hoy added inline comments to D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..
May 18 2021, 12:24 PM · Restricted Project
hoy accepted D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO.
May 18 2021, 12:08 PM · Restricted Project

May 17 2021

hoy added a comment to D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..

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

May 17 2021, 10:23 PM · Restricted Project
hoy updated the summary of D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..
May 17 2021, 10:23 PM · Restricted Project

May 16 2021

hoy updated the diff for D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..

Addressing Wenlei's comment.

May 16 2021, 11:17 PM · Restricted Project
hoy updated the summary of D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..
May 16 2021, 11:16 PM · Restricted Project
hoy committed rGf28ee1a2b386: [CSSPGO] Update pseudo probe distribution factor based on inline context. (authored by hoy).
[CSSPGO] Update pseudo probe distribution factor based on inline context.
May 16 2021, 11:12 PM
hoy closed D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..
May 16 2021, 11:12 PM · Restricted Project
hoy added a comment to D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..

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.

May 16 2021, 10:24 PM · Restricted Project
hoy updated the diff for D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..

Addressing Wenle's comments.

May 16 2021, 9:51 PM · Restricted Project
hoy added inline comments to D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..
May 16 2021, 9:50 PM · Restricted Project

May 14 2021

hoy added inline comments to D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO.
May 14 2021, 6:12 PM · Restricted Project
hoy updated the diff for D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..

Addressing Wei's comment.

May 14 2021, 5:28 PM · Restricted Project
hoy added a comment to D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one..

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.

May 14 2021, 4:42 PM · Restricted Project
hoy added reviewers for D102537: [CSSPGO] Overwrite branch weight annotated in previous pass.: wenlei, wlei, wmi, davidxl.
May 14 2021, 2:44 PM · Restricted Project
hoy requested review of D102537: [CSSPGO] Overwrite branch weight annotated in previous pass..
May 14 2021, 2:43 PM · Restricted Project
hoy added a comment to D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..

Try to understand this:

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

May 14 2021, 11:38 AM · Restricted Project
hoy added inline comments to D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..
May 14 2021, 10:37 AM · Restricted Project
hoy added inline comments to D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..
May 14 2021, 9:58 AM · Restricted Project

May 13 2021

hoy added a comment to D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one..

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)

May 13 2021, 4:02 PM · Restricted Project
hoy added a comment to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..

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?

May 13 2021, 1:44 PM · Restricted Project
hoy added reviewers for D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context.: wenlei, wlei, wmi, davidxl.
May 13 2021, 11:08 AM · Restricted Project
hoy requested review of D102429: [CSSPGO] Update pseudo probe distribution factor based on inline context..
May 13 2021, 11:07 AM · Restricted Project
hoy added inline comments to D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO.
May 13 2021, 12:35 AM · Restricted Project

May 12 2021

hoy added reviewers for D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.: tmsriram, dblaikie.
May 12 2021, 12:44 PM · Restricted Project