tejohnson (Teresa Johnson)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 27 2015, 11:17 AM (181 w, 4 d)

Recent Activity

Today

tejohnson added a comment to D53440: [LoopUnroll] allow customization for new-pass-manager version of LoopUnroll.

I know that this change lacks testing, yet I have no idea on how to organize the test.
I'm doing this change for our downstream usage, so there currently no use of customized loop-unroll upstream and there are no command-line controls that could trigger it in a test.

Any suggestions on how to test it?

Fri, Oct 19, 10:52 AM

Wed, Oct 17

tejohnson added a comment to D49362: [ThinLTO] Internalize read only globals.

Great! I only had a chance to take a cursory look so far (attending the llvm dev meeting), but a few questions/comments based on a very quick look this morning.

Wed, Oct 17, 8:44 PM

Tue, Oct 16

tejohnson committed rL344662: New test requires x86-registered-target.
New test requires x86-registered-target
Tue, Oct 16, 6:01 PM
tejohnson committed rL344660: [ThinLTO] Fix test to require asserts.
[ThinLTO] Fix test to require asserts
Tue, Oct 16, 5:21 PM
tejohnson committed rL344658: [ThinLTO] Add importing stats to thin link.
[ThinLTO] Add importing stats to thin link
Tue, Oct 16, 4:52 PM
tejohnson closed D53337: [ThinLTO] Add importing stats to thin link.
Tue, Oct 16, 4:52 PM
tejohnson created D53345: [ThinLTO] Split NotEligibleToImport into legality and inlinability flags.
Tue, Oct 16, 4:07 PM
tejohnson created D53337: [ThinLTO] Add importing stats to thin link.
Tue, Oct 16, 12:25 PM
tejohnson committed rL344631: [LTO] Call InitLLVM from llvm-lto2.
[LTO] Call InitLLVM from llvm-lto2
Tue, Oct 16, 10:39 AM
tejohnson closed D53330: [LTO] Call InitLLVM from llvm-lto2.
Tue, Oct 16, 10:39 AM
tejohnson created D53330: [LTO] Call InitLLVM from llvm-lto2.
Tue, Oct 16, 9:57 AM
tejohnson added inline comments to D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported.
Tue, Oct 16, 8:08 AM
tejohnson added a comment to D53234: Don't remove COMDATs when internalizing global objects.

Normally once they are both internalized and removed from the comdat, it shouldn't matter if only one is kept by GlobalDCE. The other is simply an internal function and no longer part of a comdat (so it's not like the comdat is incomplete).

My understanding is that deleting the COMDAT is invalid.

Tue, Oct 16, 6:45 AM
tejohnson added inline comments to D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported.
Tue, Oct 16, 6:18 AM

Mon, Oct 15

tejohnson added a comment to D52904: [hot-cold-split] fix static analysis of cold regions.

This LGTM, but please wait for @brzycki

Mon, Oct 15, 2:31 PM
tejohnson added a comment to D53234: Don't remove COMDATs when internalizing global objects.

My reading of that commit (D10679), and looking at the code, is that GlobalDCE would be able to remove these members because they should *all* be removed from the comdat by internalization. The change appears to make it such that internalization cannot proceed if any member of the comdat is externally visible and therefore cannot be internalized. If all members of the comdat can be internalized (i.e. not in the ExternalComdats set), then they can all be internalized and the comdat removed from all members.

The issue only occurs when a pass like GlobalDCE runs after Internalizer. Since the COMDAT has been removed, GlobalDCE might remove only one member from the (now deleted) COMDAT, leaving other members intact.
I've added a testcase demonstrating this.

Mon, Oct 15, 1:52 PM
tejohnson added a comment to D53294: [ThinLTO] Add an option to disable (thin)lto internalization..

Code looks fine now but just realized there is no test. You can probably create one test with a function that can normally be internalized, and then try it for the 4 combinations of LTO/ThinLTO and new/old LTO with and without your option. See test/ThinLTO/X86/internalize.ll for a ThinLTO-specific internalization test that tests both the old (via llvm-lto) and the new (via llvm-lto2) LTO APIs.

Mon, Oct 15, 1:38 PM
tejohnson added a comment to D52904: [hot-cold-split] fix static analysis of cold regions.

Are both fixes necessary to fix the issue (the one for back propagation and the one to bail out if the entry block is cold), or is either one sufficient? The patch description only mentions the former.

Mon, Oct 15, 1:11 PM
tejohnson added inline comments to D53294: [ThinLTO] Add an option to disable (thin)lto internalization..
Mon, Oct 15, 12:59 PM
tejohnson added a comment to D53294: [ThinLTO] Add an option to disable (thin)lto internalization..

The ThinLTO code is shared between the new and old LTO APIs (via thinLTOInternalizeAndPromoteInIndex), but the regular LTO internalization is not. Suggest sharing this option with lib/LTO/LTOCodeGenerator.cpp and guarding the regular LTO internalization there too to cover the old LTO API case.

Mon, Oct 15, 11:40 AM
tejohnson added inline comments to D52904: [hot-cold-split] fix static analysis of cold regions.
Mon, Oct 15, 11:36 AM
tejohnson added a comment to D53267: [CodeExtractor] Erase debug intrinsics in outlined thunks.
In D53267#1265681, @vsk wrote:
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

I should add that in the one case I looked at, the first argument to the outlined llvm.dbg.value was not updated correctly, which seems a little different than the failure mode here. So there might be multiple issues with these outlined intrinsics that need fixing. Not sure if you want to update the comment.

Hm, I think that's the same failure mode I saw. A dbg.value from the original function was moved to the outlined function, but its ValueAsMetadata operand (the "function-local metadata") wasn't updated to point to an Argument in the outlined function.

Mon, Oct 15, 11:27 AM
tejohnson added a comment to D53267: [CodeExtractor] Erase debug intrinsics in outlined thunks.
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Thanks for the fix. I didn't see this patch before I updated that bug. This should fix my issue, which related to some llvm.dbg.value intrinsics in outlined code that weren't updated properly.

Mon, Oct 15, 11:21 AM
tejohnson added a comment to D53267: [CodeExtractor] Erase debug intrinsics in outlined thunks.
In D53267#1265669, @vsk wrote:

Does this patch fix https://bugs.llvm.org/show_bug.cgi?id=22900
In which case you may want to add the testcases from that bug.

It does. I'm not sure that there's extra value in including the attached test case -- it uses an old textual IR format that's hard to update, and reduces to something similar to the test in this patch.

Fwiw I've tested this patch by building several internal frameworks and running Csmith over the weekend. I found no regressions. With Csmith, the testing method was to run -Os -g -mllvm -hot-cold-split={true, false} and check that the two CRCs were the same.

Mon, Oct 15, 11:17 AM
tejohnson added inline comments to D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported.
Mon, Oct 15, 10:22 AM
tejohnson added a reviewer for D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported: tejohnson.
Mon, Oct 15, 10:09 AM
tejohnson added inline comments to D52966: [Merge SImilar Function ThinLTO 3/n] Add hash code to function summary.
Mon, Oct 15, 10:09 AM
tejohnson added a reviewer for D52966: [Merge SImilar Function ThinLTO 3/n] Add hash code to function summary: tejohnson.
Mon, Oct 15, 10:08 AM
tejohnson added a reviewer for D52896: MergeSimilarFunctions 1/n: a code size pass to merge functions with small differences: tejohnson.
Mon, Oct 15, 10:07 AM
tejohnson added a comment to D53253: [Merge SImilar Function ThinLTO 4/n] Make merge function decisions before the thin-lto stage.

It's hard to review the changes the way they are currently split up. E.g. This patch adds some fields to the index, without using them or testing them. Those changes are in a different patch or two. It would be good to isolate all the changes that create and set these new index fields and test them via dumping the index to assembly and looking for expected values. Then have another patch that uses them in the FunctionImporter. Etc.

Mon, Oct 15, 10:06 AM
tejohnson added a reviewer for D53253: [Merge SImilar Function ThinLTO 4/n] Make merge function decisions before the thin-lto stage: tejohnson.
Mon, Oct 15, 9:55 AM
tejohnson accepted D53123: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.

LGTM

Mon, Oct 15, 8:56 AM
tejohnson added a comment to D53234: Don't remove COMDATs when internalizing global objects.

However, Internalizer currently removes non-externally-visible COMDATs when internalizing global objects. The commit which added this behavior https://github.com/llvm-mirror/llvm/commit/4a6d99b0a96d4eb27d89c22e33981ff0344c5737 states that passes like GlobalDCE can legally remove individual COMDAT members.

Mon, Oct 15, 7:53 AM

Thu, Oct 11

tejohnson accepted D53139: [ThinLTO] Don't import GV which contains blockaddress.

LGTM with a minor comment fix noted below.

Thu, Oct 11, 11:00 AM
tejohnson added inline comments to D53139: [ThinLTO] Don't import GV which contains blockaddress.
Thu, Oct 11, 8:33 AM

Wed, Oct 10

tejohnson accepted D53122: Polish the comments of cache.ll.

LGTM

Wed, Oct 10, 7:23 PM
tejohnson accepted D52836: [LTO] Account for overriding lib calls via the alias attribute.

As discussed on bug, this is already fixed in new LTO API so best to be consistent, regardless of whether users should build with -fno-builtins.

Wed, Oct 10, 2:53 PM
tejohnson accepted D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.

LGTM

Wed, Oct 10, 8:37 AM
tejohnson added a reviewer for D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy: ruiu.
Wed, Oct 10, 8:36 AM
tejohnson added inline comments to D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.
Wed, Oct 10, 6:46 AM

Tue, Oct 9

tejohnson added inline comments to D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.
Tue, Oct 9, 9:38 AM

Mon, Oct 8

tejohnson accepted D52893: [ThinLTO] Keep non-prevailing (linkonce|weak)_odr symbols live.

LGTM

Mon, Oct 8, 8:09 AM

Fri, Oct 5

tejohnson added a comment to D52893: [ThinLTO] Keep non-prevailing (linkonce|weak)_odr symbols live.

This seems reasonable - we should be able to keep ODR non-prevailing defs around to optimize with. I assume this is a missed optimization fix and not a correctness fix (i.e. the build currently succeeds and builds a correct binary)? Question about that below.

Fri, Oct 5, 10:07 AM
tejohnson added a comment to D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.

Can you upload the patch with context? See the following for instructions:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Fri, Oct 5, 9:58 AM
tejohnson added a comment to D52836: [LTO] Account for overriding lib calls via the alias attribute.

I'll respond more on the bug (especially since I can't reproduce the actual link time error), but I think the right way to fix these types of issues is to compile with one of -ffreestanding, -fno-builtin, or -fno-builtin-log (or whatever is being overridden), so that clang doesn't translate the call to an intrinsic. Clang translates to the intrinsic assuming it is calling the builtin, which means it can assume specific semantics for the call, and there is nothing stopping the compiler from translating that intrinsic call to e.g. an inline expansion of the function (that currently happens e.g. for memcpy and some other builtins).

Fri, Oct 5, 8:22 AM

Tue, Oct 2

tejohnson added a comment to D52708: Add support for new pass manager.

LGTM as well, provided @tejohnson 's test complete successfully.

Tue, Oct 2, 10:16 AM
tejohnson accepted D52708: Add support for new pass manager.

LGTM with one minor fix noted below. Thanks! I will give it a shot with our apps which use the new PM.

Tue, Oct 2, 8:24 AM

Mon, Oct 1

tejohnson added a comment to D52708: Add support for new pass manager.

Please add a testcase that will exercise the new flag.

Mon, Oct 1, 7:45 AM

Thu, Sep 27

tejohnson committed rL343226: [WPD] Fix incorrect devirtualization after indirect call promotion.
[WPD] Fix incorrect devirtualization after indirect call promotion
Thu, Sep 27, 7:57 AM
tejohnson closed D52514: [WPD] Fix incorrect devirtualization after indirect call promotion.
Thu, Sep 27, 7:57 AM

Wed, Sep 26

tejohnson added a comment to D50658: Hot cold splitting pass.

I see that there is a new Pass Manager version of your new pass, but I don't see that it is ever being enabled in the new pass manager pipeline (or tested). Are you planning to add that?

Wed, Sep 26, 1:31 PM

Tue, Sep 25

tejohnson committed rL343021: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
[ThinLTO] Efficiency fix for writing type id records in per-module indexes
Tue, Sep 25, 1:16 PM
tejohnson closed D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Tue, Sep 25, 1:16 PM
tejohnson created D52514: [WPD] Fix incorrect devirtualization after indirect call promotion.
Tue, Sep 25, 12:31 PM
tejohnson updated the diff for D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.

Use multimap

Tue, Sep 25, 11:54 AM
tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Tue, Sep 25, 11:54 AM

Mon, Sep 24

tejohnson added a comment to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.

ping

Mon, Sep 24, 7:29 AM

Fri, Sep 21

tejohnson accepted D52383: Fix codemodels.c test case (only test mcmodel-kernel on x86).

LGTM

Fri, Sep 21, 4:17 PM
tejohnson accepted D52323: Add necessary support for storing code-model to module IR..

LGTM

Fri, Sep 21, 10:44 AM
tejohnson accepted D52322: Pass code-model through Module IR to LTO which will use is.

LGTM with a minor change suggested.

Fri, Sep 21, 10:44 AM
tejohnson added inline comments to D52322: Pass code-model through Module IR to LTO which will use is.
Fri, Sep 21, 10:01 AM
tejohnson added a comment to D52323: Add necessary support for storing code-model to module IR..

Note that if you add a line like:

Fri, Sep 21, 8:29 AM
tejohnson added inline comments to D52322: Pass code-model through Module IR to LTO which will use is.
Fri, Sep 21, 8:23 AM
tejohnson accepted D52354: Fix some missing opcodes in bcanalyzer.

LGTM

Fri, Sep 21, 6:48 AM

Thu, Sep 20

tejohnson accepted D52319: [GlobalDCE] AvailableExternal linkage is checked in isDiscardableIfUnused [NFC]..

LGTM

Thu, Sep 20, 1:17 PM
tejohnson added a comment to D52319: [GlobalDCE] AvailableExternal linkage is checked in isDiscardableIfUnused [NFC]..

I think this is an NFC (no functional change) cleanup, right? It would be good to add that in the description - i.e. put "[NFC]" at the end of the summary first line.

Thu, Sep 20, 12:56 PM
tejohnson added a comment to D50658: Hot cold splitting pass.

I see that there is a new Pass Manager version of your new pass, but I don't see that it is ever being enabled in the new pass manager pipeline (or tested). Are you planning to add that?

Thu, Sep 20, 7:03 AM

Sep 19 2018

tejohnson accepted D52201: [ThinLTO] Write TYPE_IDs for types used in functions imported by aliases.

LGTM

Sep 19 2018, 11:36 AM
tejohnson accepted D52175: [WholeProgramDevirt] Don't process declarations when building type id map.

In your summary, I think you mean it can copy metadata (not attributes)?

Sep 19 2018, 8:29 AM
tejohnson accepted D52203: [ThinLTO] Extract getReferencedTypeIds from [NFC].

LGTM

Sep 19 2018, 7:19 AM
tejohnson added a comment to D52201: [ThinLTO] Write TYPE_IDs for types used in functions imported by aliases.

Can you update description to point out that this is an issue because we will import the alias as a copy of aliasee?

Sep 19 2018, 7:17 AM

Sep 18 2018

tejohnson committed rL342479: [ThinLTO] Update LangRef doc for summary parsing.
[ThinLTO] Update LangRef doc for summary parsing
Sep 18 2018, 6:45 AM
tejohnson closed D51540: [ThinLTO] Update LangRef doc for summary parsing.
Sep 18 2018, 6:45 AM
tejohnson committed rL342477: [LTO] Make detection of WPD remark enablement more robust.
[LTO] Make detection of WPD remark enablement more robust
Sep 18 2018, 6:44 AM
tejohnson closed D51556: [LTO] Make detection of WPD remark enablement more robust.
Sep 18 2018, 6:43 AM

Sep 17 2018

tejohnson added a comment to D51556: [LTO] Make detection of WPD remark enablement more robust.

ping

Sep 17 2018, 8:23 AM
tejohnson added a comment to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.

ping

Sep 17 2018, 8:22 AM
tejohnson added a comment to D51540: [ThinLTO] Update LangRef doc for summary parsing.

ping

Sep 17 2018, 8:22 AM

Sep 14 2018

tejohnson accepted D52049: [ThinLTOCodeGenerator] Avoid Rehash StringMap in ThreadPool.

LGTM with nit.

Sep 14 2018, 9:41 AM

Sep 13 2018

tejohnson added inline comments to D52023: [ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values.
Sep 13 2018, 6:58 AM
tejohnson added inline comments to D52023: [ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values.
Sep 13 2018, 6:26 AM

Sep 7 2018

tejohnson added inline comments to D51744: [WIP] Early ThinLTOLayer2 prototype.
Sep 7 2018, 10:14 AM

Sep 4 2018

tejohnson updated the diff for D51540: [ThinLTO] Update LangRef doc for summary parsing.

Split into 2 paragraphs as suggested

Sep 4 2018, 2:09 PM
tejohnson updated the diff for D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.

Change to use a map to a vector of matching type id, summary pairs

Sep 4 2018, 2:06 PM
tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Sep 4 2018, 2:05 PM

Aug 31 2018

tejohnson created D51556: [LTO] Make detection of WPD remark enablement more robust.
Aug 31 2018, 1:37 PM
tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 31 2018, 12:54 PM
tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 31 2018, 11:36 AM
tejohnson updated the diff for D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.

Change the TypeIdMap in summary to be indexed by GUID.

Aug 31 2018, 10:30 AM
tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 31 2018, 10:30 AM
tejohnson created D51540: [ThinLTO] Update LangRef doc for summary parsing.
Aug 31 2018, 7:06 AM
tejohnson added a comment to D51497: [WIP][ORC][ThinLTO] Early ThinLTO-JIT prototype and basic tests for next-gen ORC APIs.

You should check in LLVM textual IR files (.ll) instead of C++ source files or object files. Using clang -S -emit-llvm (or llvm-dis some-thinlto-bitcode-file.o) should get you started.

Yes that would be great, but wouldn't it require IR representation for summaries? I didn't double-check myself, but there's a note in the language ref, "that temporarily the summary entries are skipped when parsing the assembly". Not sure if I understand that correctly, but I was under the impression that we can't have summaries in IR files yet.
https://llvm.org/docs/LangRef.html#thinlto-summary

Aug 31 2018, 6:56 AM

Aug 30 2018

tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 30 2018, 2:05 PM
tejohnson added a comment to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
In D51330#1218260, @pcc wrote:

Did you consider changing the type id mapping data structure in ModuleSummaryIndex instead of creating a new one? Since we now use a mapping from guids in several places it seems sensible to do that. I think that it would work to change the type of TypeIdMap to something like:

DenseMap<GlobalValue::GUID, TinyPtrVector<std::pair<std::string, TypeIdSummary>>>
Aug 30 2018, 8:21 AM

Aug 27 2018

tejohnson added inline comments to D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 27 2018, 4:44 PM
tejohnson created D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes.
Aug 27 2018, 4:40 PM

Aug 24 2018

tejohnson accepted D50435: [Inliner] Attribute callsites with inline remarks.

LGTM

Aug 24 2018, 7:18 AM

Aug 23 2018

tejohnson accepted D51198: [LTO] Fix -save-temps with LTO and unnamed globals..

LGTM

Aug 23 2018, 5:56 PM
tejohnson added a comment to D51198: [LTO] Fix -save-temps with LTO and unnamed globals..

Thanks. Can you fix the same code in EmitAssemblyWithNewPassManager?

Aug 23 2018, 4:59 PM

Aug 21 2018

tejohnson accepted D51060: [gold] -thinlto-object-suffix-replace: don't append new suffix if path does not end with old suffix.

LGTM. Thanks for the cleanup!

Aug 21 2018, 6:40 PM