tejohnson (Teresa Johnson)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 27 2015, 11:17 AM (133 w, 6 d)

Recent Activity

Tue, Nov 14

tejohnson accepted D40056: [ThinLTO] Remove too aggressive assertion in building function call graph..

LGTM
Thanks!

Tue, Nov 14, 6:36 PM

Mon, Nov 13

tejohnson committed rL318042: [ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang.
[ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang
Mon, Nov 13, 7:40 AM
tejohnson added a reviewer for D39921: [SROA] Correctly invalidate analyses when dead instructions deleted: chandlerc.
Mon, Nov 13, 7:01 AM

Fri, Nov 10

tejohnson committed rL317959: Revert "[ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang".
Revert "[ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang"
Fri, Nov 10, 5:09 PM
tejohnson committed rL317952: Add x86-registered-target to REQUIRES for new test.
Add x86-registered-target to REQUIRES for new test
Fri, Nov 10, 4:07 PM
tejohnson committed rL317951: [ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang.
[ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang
Fri, Nov 10, 3:38 PM
tejohnson closed D39923: [ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang by committing rL317951: [ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang.
Fri, Nov 10, 3:37 PM
tejohnson created D39923: [ThinLTO] Handle -fdebug-pass-manager for backend invocations via clang.
Fri, Nov 10, 2:10 PM
tejohnson created D39921: [SROA] Correctly invalidate analyses when dead instructions deleted.
Fri, Nov 10, 1:29 PM
tejohnson created D39901: ASAN lit support for LTO testing and add back ThinLTO test.
Fri, Nov 10, 7:42 AM

Thu, Nov 9

tejohnson committed rL317823: Revert new ThinLTO ASAN test until lit support added.
Revert new ThinLTO ASAN test until lit support added
Thu, Nov 9, 11:27 AM

Wed, Nov 8

tejohnson added a comment to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
In D37993#918921, @pcc wrote:
In D37993#912428, @pcc wrote:
In D37993#893466, @pcc wrote:

Is it a good idea to support cache pruning in the gold plugin? As I mentioned in D31063 there are some unavoidable race conditions due to limitations in the plugin API.

Ping... not sure whether it's a good idea to ship a known racy feature.

If you really want this feature in gold I'd suggest extending the gold plugin API so that it accepts file descriptors, and then have the plugin support this feature conditionally on whether the gold version is new enough.

Hi Peter, thanks for pointing this out, and sorry for the delay in responding. Wondering whether instead of removing cache pruning support from gold-plugin for now, we could do something else: write the MemoryBuffer to another temp file and use that path (setting IsTemporary to true so it gets cleaned up). This would obviously require more space on disk, but maybe worth it to enable the pruning safely.

Copying to a temporary file sounds fine to me. And if gold ever gets support for file descriptors we could always use the copying code path as a fallback for older versions.

Wed, Nov 8, 10:12 PM
tejohnson committed rL317728: [ThinLTO] New test needs to require LTO.
[ThinLTO] New test needs to require LTO
Wed, Nov 8, 1:48 PM
tejohnson committed rL317723: [ThinLTO] Ensure sanitizer passes are run.
[ThinLTO] Ensure sanitizer passes are run
Wed, Nov 8, 12:28 PM
tejohnson committed rL317717: Revert "[ThinLTO] Ensure sanitizer passes are run".
Revert "[ThinLTO] Ensure sanitizer passes are run"
Wed, Nov 8, 12:09 PM
tejohnson committed rL317715: [ThinLTO] Ensure sanitizer passes are run.
[ThinLTO] Ensure sanitizer passes are run
Wed, Nov 8, 11:47 AM
tejohnson closed D39566: [ThinLTO] Ensure sanitizer passes are run by committing rL317715: [ThinLTO] Ensure sanitizer passes are run.
Wed, Nov 8, 11:47 AM
tejohnson committed rL317714: [ThinLTO] Ensure sanitizer passes are run.
[ThinLTO] Ensure sanitizer passes are run
Wed, Nov 8, 11:46 AM
tejohnson closed D39565: [ThinLTO] Ensure sanitizer passes are run by committing rL317714: [ThinLTO] Ensure sanitizer passes are run.
Wed, Nov 8, 11:46 AM
tejohnson added a comment to D39565: [ThinLTO] Ensure sanitizer passes are run.

ping

Wed, Nov 8, 7:08 AM

Tue, Nov 7

tejohnson added a comment to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

I'm confused now: was there or not a reference edges for the internal function before this patch? If yes it seems strange that the issue would be in ld64 because the imported function would reference the renamed function, how could it match the external original f()?

Tue, Nov 7, 8:43 PM
tejohnson added a comment to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.
In D37993#912428, @pcc wrote:
In D37993#893466, @pcc wrote:

Is it a good idea to support cache pruning in the gold plugin? As I mentioned in D31063 there are some unavoidable race conditions due to limitations in the plugin API.

Ping... not sure whether it's a good idea to ship a known racy feature.

If you really want this feature in gold I'd suggest extending the gold plugin API so that it accepts file descriptors, and then have the plugin support this feature conditionally on whether the gold version is new enough.

Tue, Nov 7, 8:25 PM
tejohnson accepted D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

Presumably @f is added to the references of @ext's FunctionSummary?

Sorry, I've misunderstood you. @ext seems to be totally fine. It has @f as a call edge and not as a reference (because of isCallee() check). The problem is in @main. Without my change it has @ext only as a reference, but not as call edge. With my change it has it both as reference and as call edge.

Tue, Nov 7, 7:27 PM
tejohnson added a comment to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

Tue, Nov 7, 2:01 PM

Mon, Nov 6

tejohnson added a comment to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Mon, Nov 6, 1:50 PM

Sat, Nov 4

tejohnson added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Sat, Nov 4, 7:49 AM

Thu, Nov 2

tejohnson added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Thu, Nov 2, 5:25 PM
tejohnson created D39566: [ThinLTO] Ensure sanitizer passes are run.
Thu, Nov 2, 1:33 PM
tejohnson added a dependent revision for D39565: [ThinLTO] Ensure sanitizer passes are run: D39566: [ThinLTO] Ensure sanitizer passes are run.
Thu, Nov 2, 1:33 PM
tejohnson created D39565: [ThinLTO] Ensure sanitizer passes are run.
Thu, Nov 2, 1:32 PM
tejohnson added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Thu, Nov 2, 1:25 PM

Wed, Nov 1

tejohnson accepted D38763: Include already promoted counts when computing SUM for VP..

LGTM
Sorry, completely missed this one earlier.

Wed, Nov 1, 9:42 PM
tejohnson accepted D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported..

LGTM

Wed, Nov 1, 11:47 AM
tejohnson added inline comments to D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported..
Wed, Nov 1, 11:31 AM
tejohnson accepted D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..

LGTM

Wed, Nov 1, 10:52 AM
tejohnson added a comment to D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported..

Does VP metadata get synthesized from the SamplePGO profile? The original bug report had some of the targets on VP metadata attached to the indirect call, but was missing others. Why not include all the targets on that VP metadata rather than marking them for import which seems more hacky.

Because we need to handle the indirect call chain too. E.g. foo() indirectly calls bar(), which indirectly calls baz(). Assuming these 2 calls are ICPed and inlined in the profiling binary, when processing foo's indirect callsite, marking bar's GUID in the VP is not enough, but adding baz's GUID to foo's callsite does not make sense as baz is not its call target. That's why we need to keep the transitive closure as function level metadata.

But the case here is when they are in fact in the same module. I'm assuming the situation you describe here already works when bar and baz are in different modules than foo. In this case we have foo and bar in the same module, and were missing the VP metadata showing that foo calls bar indirectly. If baz is also in the same module then the same situation applies to the callsite in bar. If it is in a different module then wouldn't the existing logic handle it? I.e. it would be on bar's function entry metadata?

If baz is in the same module as bar and foo, then we need to have baz in foo's entry metadata. For the callsite in bar (standalone copy), it is possible that bar becomes cold after inlined into foo thus there is no profile to indicate bar has an indirect call to baz.

If baz is in another module, then in the current implementation (without this patch), it will be added in foo's entry metadata.

Wed, Nov 1, 10:23 AM
tejohnson added inline comments to D39484: LTO: Apply global DCE to ThinLTO modules at LTO opt level 0..
Wed, Nov 1, 7:52 AM

Tue, Oct 31

tejohnson added a comment to D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported..

Does VP metadata get synthesized from the SamplePGO profile? The original bug report had some of the targets on VP metadata attached to the indirect call, but was missing others. Why not include all the targets on that VP metadata rather than marking them for import which seems more hacky.

Because we need to handle the indirect call chain too. E.g. foo() indirectly calls bar(), which indirectly calls baz(). Assuming these 2 calls are ICPed and inlined in the profiling binary, when processing foo's indirect callsite, marking bar's GUID in the VP is not enough, but adding baz's GUID to foo's callsite does not make sense as baz is not its call target. That's why we need to keep the transitive closure as function level metadata.

Tue, Oct 31, 3:30 PM
tejohnson added a comment to D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported..

Does VP metadata get synthesized from the SamplePGO profile? The original bug report had some of the targets on VP metadata attached to the indirect call, but was missing others. Why not include all the targets on that VP metadata rather than marking them for import which seems more hacky.

Tue, Oct 31, 2:58 PM
tejohnson committed rL316996: [ThinLTO] Double bits of module hash used for renaming.
[ThinLTO] Double bits of module hash used for renaming
Tue, Oct 31, 5:56 AM
tejohnson closed D39443: [ThinLTO] Double bits of module hash used for renaming by committing rL316996: [ThinLTO] Double bits of module hash used for renaming.
Tue, Oct 31, 5:56 AM
tejohnson updated the diff for D39443: [ThinLTO] Double bits of module hash used for renaming.

Address comment

Tue, Oct 31, 5:56 AM

Mon, Oct 30

tejohnson created D39443: [ThinLTO] Double bits of module hash used for renaming.
Mon, Oct 30, 7:11 PM
tejohnson added inline comments to D39356: [ThinLTO] Fix missing call graph edges for calls with bitcasts..
Mon, Oct 30, 1:30 PM

Tue, Oct 24

tejohnson committed rL316544: [ThinLTO] Make test for promoted names more specific.
[ThinLTO] Make test for promoted names more specific
Tue, Oct 24, 8:42 PM

Oct 13 2017

tejohnson added inline comments to D36850: [ThinLTO] Add norecurse function attribute propagation.
Oct 13 2017, 1:08 PM
tejohnson added inline comments to D36850: [ThinLTO] Add norecurse function attribute propagation.
Oct 13 2017, 12:20 PM
tejohnson added a comment to D36311: [ThinLTO] Add GraphTraits for FunctionSummaries.

Just minor comments left at this point, I noted a few places where you hadn't yet addressed my previous commenting nits, plus a question about paring down the new operators, and some suggestions for minorly beefing up the matching you are doing on the test lines.

Oct 13 2017, 10:39 AM
tejohnson added a comment to D36311: [ThinLTO] Add GraphTraits for FunctionSummaries.

Any updates on this @tejohnson @davide ?

Oct 13 2017, 8:38 AM

Oct 10 2017

tejohnson accepted D38478: Use the first instruction's count to estimate the funciton's entry frequency..

LGTM

Oct 10 2017, 1:19 PM
tejohnson added inline comments to D38478: Use the first instruction's count to estimate the funciton's entry frequency..
Oct 10 2017, 9:41 AM

Oct 9 2017

tejohnson added inline comments to D38478: Use the first instruction's count to estimate the funciton's entry frequency..
Oct 9 2017, 8:06 AM

Oct 6 2017

tejohnson added inline comments to D38603: Directly return promoted direct call instead of rely on stripPointerCast..
Oct 6 2017, 9:01 AM
tejohnson accepted D38603: Directly return promoted direct call instead of rely on stripPointerCast..

LGTM although I have a question on test

Oct 6 2017, 8:25 AM

Oct 5 2017

tejohnson accepted D38477: Annotate VP prof on indirect call if it is ICPed in the profiled binary..

LGTM
(please just fix the 2 typos in the patch summary before submitting)

Oct 5 2017, 11:16 AM

Oct 4 2017

tejohnson accepted D38517: Enabling new pass manager in LTO (and thinLTO) link step via -fexperimental-new-pass-manager option.

LGTM
thanks!

Oct 4 2017, 8:56 AM

Oct 3 2017

tejohnson added a comment to D38517: Enabling new pass manager in LTO (and thinLTO) link step via -fexperimental-new-pass-manager option.

Thanks for adding this. Please add a test like the one for -ffunction-sections (tools/clang/test/Driver/gold-lto-sections.c).

Oct 3 2017, 5:14 PM
tejohnson added a comment to D38478: Use the first instruction's count to estimate the funciton's entry frequency..

Started looking at, but have a few initial questions.

Oct 3 2017, 11:32 AM
tejohnson added a comment to D38477: Annotate VP prof on indirect call if it is ICPed in the profiled binary..

Summary has a couple of typos

Oct 3 2017, 11:22 AM

Sep 30 2017

tejohnson accepted D38094: Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases..

lgtm

Sep 30 2017, 4:03 PM
tejohnson added inline comments to D38094: Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases..
Sep 30 2017, 2:36 PM
tejohnson added a comment to D38094: Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases..

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.

Sep 30 2017, 12:05 PM

Sep 29 2017

tejohnson committed rL314527: [ThinLTO] Use decimal suffix for promoted values to match demanglers.
[ThinLTO] Use decimal suffix for promoted values to match demanglers
Sep 29 2017, 8:57 AM
tejohnson closed D38405: [ThinLTO] Use decimal suffix for promoted values to match demanglers by committing rL314527: [ThinLTO] Use decimal suffix for promoted values to match demanglers.
Sep 29 2017, 8:57 AM
tejohnson created D38405: [ThinLTO] Use decimal suffix for promoted values to match demanglers.
Sep 29 2017, 8:08 AM

Sep 20 2017

tejohnson committed rL313766: [ThinLTO] Fix dead stripping analysis for SamplePGO.
[ThinLTO] Fix dead stripping analysis for SamplePGO
Sep 20 2017, 10:11 AM
tejohnson closed D38086: [ThinLTO] Fix dead stripping analysis for SamplePGO by committing rL313766: [ThinLTO] Fix dead stripping analysis for SamplePGO.
Sep 20 2017, 10:11 AM
tejohnson created D38086: [ThinLTO] Fix dead stripping analysis for SamplePGO.
Sep 20 2017, 9:35 AM

Sep 19 2017

tejohnson accepted D36637: Import all inlined indirect call targets for SamplePGO..

LGTM

Sep 19 2017, 1:58 PM
tejohnson added inline comments to D36637: Import all inlined indirect call targets for SamplePGO..
Sep 19 2017, 12:07 PM
tejohnson accepted D38018: Handle profile mismatch correctly for SamplePGO..

LGTM

Sep 19 2017, 11:03 AM
tejohnson added inline comments to D38018: Handle profile mismatch correctly for SamplePGO..
Sep 19 2017, 10:12 AM

Sep 18 2017

tejohnson accepted D37995: [Docs] Document cache pruning support for gold.

LGTM with one fix noted.

Sep 18 2017, 4:21 PM · Restricted Project
tejohnson accepted D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.

LGTM

Sep 18 2017, 4:20 PM
tejohnson added a comment to D37993: [ThinLTO/gold] Implement ThinLTO cache pruning support.

Thanks for working on adding this missing feature! Please add a test and include an update to the info on where cache pruning is supported in ThinLTO.rst (for examples of both, see https://reviews.llvm.org/D37607, where it was added to lld).

Sep 18 2017, 1:17 PM

Sep 13 2017

tejohnson accepted D37842: Reland r313157, "ThinLTO: Correctly follow aliasee references when dead stripping." which was reverted in r313222..

LGTM

Sep 13 2017, 9:56 PM
tejohnson committed rL313158: [ThinLTO] AliasSummary should not have any references.
[ThinLTO] AliasSummary should not have any references
Sep 13 2017, 10:12 AM
tejohnson closed D37814: [ThinLTO] AliasSummary should not have any references by committing rL313158: [ThinLTO] AliasSummary should not have any references.
Sep 13 2017, 10:12 AM
tejohnson created D37814: [ThinLTO] AliasSummary should not have any references.
Sep 13 2017, 9:45 AM
tejohnson committed rL313153: Fix bot failures by requiring x86 target in new test.
Fix bot failures by requiring x86 target in new test
Sep 13 2017, 8:37 AM
tejohnson committed rL313151: [ThinLTO] For SamplePGO, need to handle ICP targets consistently in thin link.
[ThinLTO] For SamplePGO, need to handle ICP targets consistently in thin link
Sep 13 2017, 8:18 AM
tejohnson closed D37783: [ThinLTO] For SamplePGO, need to handle ICP targets consistently in thin link by committing rL313151: [ThinLTO] For SamplePGO, need to handle ICP targets consistently in thin link.
Sep 13 2017, 8:18 AM
tejohnson accepted D37789: ThinLTO: Correctly follow aliasee references when dead stripping..

LGTM, not sure how we didn't hit this before!

Sep 13 2017, 8:18 AM

Sep 12 2017

tejohnson created D37783: [ThinLTO] For SamplePGO, need to handle ICP targets consistently in thin link.
Sep 12 2017, 6:21 PM

Sep 6 2017

tejohnson accepted D37553: Analysis: Correctly handle all function operand references..

LGTM

Sep 6 2017, 10:09 PM

Sep 1 2017

tejohnson accepted D37370: ModuleSummaryAnalysis: Correctly handle refs from function inline asm to module inline asm..

LGTM too other than test target dependent issue Mehdi pointed out.

Sep 1 2017, 7:07 AM

Aug 29 2017

tejohnson committed rL312019: [ThinLTO] Clean up stale alias import handling.
[ThinLTO] Clean up stale alias import handling
Aug 29 2017, 11:19 AM
tejohnson closed D37266: [ThinLTO] Clean up stale alias import handling by committing rL312019: [ThinLTO] Clean up stale alias import handling.
Aug 29 2017, 11:19 AM
tejohnson added a comment to D37266: [ThinLTO] Clean up stale alias import handling.

Is it worth having the DEBUG output if no aliases are ever imported?

Aug 29 2017, 11:09 AM
tejohnson created D37266: [ThinLTO] Clean up stale alias import handling.
Aug 29 2017, 10:33 AM
tejohnson accepted D37252: Add null check for promoted direct call.

LGTM

Aug 29 2017, 6:18 AM

Aug 25 2017

tejohnson added a comment to D36311: [ThinLTO] Add GraphTraits for FunctionSummaries.

This looks really good, just a few minor comments. But I do have a concern about the test and it not showing the multiple node SCCs that I would expect. Can you investigate?

Aug 25 2017, 9:00 AM
tejohnson accepted D37144: [gold] Fix up a new test to allow it to pass on non x86 builds..

It would be great to have a bot that catches these.

Aug 25 2017, 6:42 AM

Aug 23 2017

tejohnson added a comment to D36311: [ThinLTO] Add GraphTraits for FunctionSummaries.

Getting there, thanks!

Aug 23 2017, 7:54 AM

Aug 22 2017

tejohnson added a comment to D36987: [ThinLTO} Add modref information to call info in function summary.

Thanks for doing this. Some comments below, but I'd like someone else like Davide or Mehdi to review since I wrote some of this.

Aug 22 2017, 1:22 PM

Aug 21 2017

tejohnson added a comment to D36910: [ThinLTO] WIP Tracking of per-call mod ref info.

I added bitcode read/write support and updated all the tests so they use the new bitcode. I was planning on waiting for a bit more feedback on this patch (and updating mine with the requested changes), but I can post mine whenever you want to move the discussion to llvm-commits.

Aug 21 2017, 1:19 PM
tejohnson accepted D36834: [lib/Analysis] - Mark personality functions as live..

LGTM

Aug 21 2017, 9:02 AM
tejohnson added a comment to D36850: [ThinLTO] Add norecurse function attribute propagation.

Looks really close to me, just some comments on the dummy root node detection. But note I had some comments on D36311 that need to be resolved.

Aug 21 2017, 6:44 AM

Aug 19 2017

tejohnson added a comment to D36910: [ThinLTO] WIP Tracking of per-call mod ref info.

I'm OK with the direction, Teresa, thanks.
If Charles wants to pursue this path before the GSoC ends, that will be great.

Yeah, I definitely like this approach better than what I had written. I can address the TODOs and clean it up a bit on Monday?

Aug 19 2017, 3:47 PM
tejohnson committed rL311257: Fix bot failures by requiring x86 target.
Fix bot failures by requiring x86 target
Aug 19 2017, 12:16 PM