Page MenuHomePhabricator

tejohnson (Teresa Johnson)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

tejohnson updated the diff for D54507: [ThinLTO] Handle chains of aliases.

Address comments

Fri, Dec 14, 2:03 PM
tejohnson accepted D55717: Add --plugin-opt=emit-llvm option..

Noting below one difference compared to gold-plugin behavior, which I think is ok, so LGTM.

Fri, Dec 14, 1:47 PM

Yesterday

tejohnson accepted D55660: [SampleFDO] handle ProfileSampleAccurate when initializing function entry count.

Nice! lgtm

Thu, Dec 13, 9:45 AM
tejohnson added inline comments to D54175: [PGO] context sensitive PGO.
Thu, Dec 13, 9:08 AM
tejohnson updated the diff for D54507: [ThinLTO] Handle chains of aliases.

Fix algorithm, update test.

Thu, Dec 13, 8:45 AM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.

New version which more closely mirrors logic proposed in D29781 comments. One difference is that I update all aliases in the chain in one recursive pass. In any case, this should make it easy to implement D29781 on top as a follow on.

Thu, Dec 13, 8:44 AM
tejohnson accepted D43521: [ThinLTO] Compute synthetic function entry count.

LGTM with 1 minor comment nit below, and also can you please add a test like test/Bitcode/thinlto-deadstrip-flag.ll to test for the new flag directly?

Thu, Dec 13, 8:39 AM

Wed, Dec 12

tejohnson added inline comments to D54507: [ThinLTO] Handle chains of aliases.
Wed, Dec 12, 6:41 PM
tejohnson added inline comments to D54507: [ThinLTO] Handle chains of aliases.
Wed, Dec 12, 6:08 PM
tejohnson updated the diff for D54507: [ThinLTO] Handle chains of aliases.

Address comments

Wed, Dec 12, 6:08 PM
tejohnson created D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases.
Wed, Dec 12, 2:17 PM
tejohnson added a child revision for D54507: [ThinLTO] Handle chains of aliases: D55620: [ThinLTO] Clang changes to utilize new pass to handle chains of aliases.
Wed, Dec 12, 2:17 PM
tejohnson updated the diff for D54507: [ThinLTO] Handle chains of aliases.

Update per comments.

Wed, Dec 12, 2:16 PM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.
In D54507#1328732, @pcc wrote:
In D54507#1327677, @pcc wrote:
In D54507#1327573, @pcc wrote:
In D54507#1327394, @pcc wrote:

Isn't this just a bug in promotion? It should probably internalize any non-prevailing aliases like lib/Linker does.

Can you clarify? The summary doesn't contain any references to the intermediate aliasee so it looks completely unreferenced.

And indeed it isn't. The important thing to keep alive is the base object, which is what the code already does.

My first idea was that we would change the code in convertToDeclaration here:
http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#923
so that we would RAUW all alias references to point to the external and all non-alias references to point to the aliasee. But unfortunately it doesn't seem easy to implement "replace some uses with" on constants.

Probably the simplest fix is to make sure that each aliasee is canonicalized to point to the base object before calling convertToDeclaration.

Do you mean make sure that each "alias" is canonicalized to point to the base object? I.e. if we have an alias relationship "A aliases B aliases C", then convert it to "A aliases C" and "B aliases C"?

Yes, exactly.

Presumably that would fix the issue, but the summary is still not accurately reflecting the IR (although not sure where else it would be problematic).

I don't think it would be problematic. In this case the summary contains the more accurate representation of reality because references to globals in alias definitions, unlike all other references, are not subject to linker preemption, and we shouldn't allow the less-accurate representation in the IR to leak anywhere else.

Philosophically, I think the summary should summarize the IR that we have (in the bitcode file), not the IR that we want. So I'm not completely comfortable with this approach - I'm actually thinking it may be better to add a skeleton alias canonicalization pass (taken from that draft alias canonicalization patch D29781) that is invoked during thinlto preparation. For now, it can just do this flatting of alias chains, and eventually expanded into the full canonicalization (and invoked in all paths as described in that patch description). I don't want to sign up to do the whole hog canonicalization currently since this bug only shows up at -O0 and I don't have the bandwidth to focus on it ATM. Does that seem like a reasonable interim approach?

Sounds reasonable enough. It seems a little better to do this during LTO because it would allow us to upgrade old bitcode but I'm happy for it to be a pass as well. I left some comments on D29781 about the overall approach there.

Wed, Dec 12, 2:16 PM
tejohnson added a comment to D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API.

Ping. Is there any comments of implementing C API in this approach?

Wed, Dec 12, 11:40 AM
tejohnson added a reviewer for D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API: pcc.
Wed, Dec 12, 11:32 AM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.
In D54507#1327677, @pcc wrote:
In D54507#1327573, @pcc wrote:
In D54507#1327394, @pcc wrote:

Isn't this just a bug in promotion? It should probably internalize any non-prevailing aliases like lib/Linker does.

Can you clarify? The summary doesn't contain any references to the intermediate aliasee so it looks completely unreferenced.

And indeed it isn't. The important thing to keep alive is the base object, which is what the code already does.

My first idea was that we would change the code in convertToDeclaration here:
http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#923
so that we would RAUW all alias references to point to the external and all non-alias references to point to the aliasee. But unfortunately it doesn't seem easy to implement "replace some uses with" on constants.

Probably the simplest fix is to make sure that each aliasee is canonicalized to point to the base object before calling convertToDeclaration.

Do you mean make sure that each "alias" is canonicalized to point to the base object? I.e. if we have an alias relationship "A aliases B aliases C", then convert it to "A aliases C" and "B aliases C"?

Yes, exactly.

Presumably that would fix the issue, but the summary is still not accurately reflecting the IR (although not sure where else it would be problematic).

I don't think it would be problematic. In this case the summary contains the more accurate representation of reality because references to globals in alias definitions, unlike all other references, are not subject to linker preemption, and we shouldn't allow the less-accurate representation in the IR to leak anywhere else.

Wed, Dec 12, 9:43 AM
tejohnson accepted D55567: [SampleFDO] Extend profile-sample-accurate option to cover isFunctionColdInCallGraph.

lgtm

Wed, Dec 12, 8:54 AM
tejohnson added inline comments to D43521: [ThinLTO] Compute synthetic function entry count.
Wed, Dec 12, 8:27 AM
tejohnson added inline comments to D55567: [SampleFDO] Extend profile-sample-accurate option to cover isFunctionColdInCallGraph.
Wed, Dec 12, 6:44 AM

Tue, Dec 11

tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.
In D54507#1327573, @pcc wrote:
In D54507#1327394, @pcc wrote:

Isn't this just a bug in promotion? It should probably internalize any non-prevailing aliases like lib/Linker does.

Can you clarify? The summary doesn't contain any references to the intermediate aliasee so it looks completely unreferenced.

And indeed it isn't. The important thing to keep alive is the base object, which is what the code already does.

My first idea was that we would change the code in convertToDeclaration here:
http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#923
so that we would RAUW all alias references to point to the external and all non-alias references to point to the aliasee. But unfortunately it doesn't seem easy to implement "replace some uses with" on constants.

Probably the simplest fix is to make sure that each aliasee is canonicalized to point to the base object before calling convertToDeclaration.

Tue, Dec 11, 2:06 PM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.
In D54507#1327394, @pcc wrote:

Isn't this just a bug in promotion? It should probably internalize any non-prevailing aliases like lib/Linker does.

Tue, Dec 11, 1:00 PM
tejohnson updated the diff for D54507: [ThinLTO] Handle chains of aliases.

Address comments

Tue, Dec 11, 12:04 PM
tejohnson added inline comments to D54507: [ThinLTO] Handle chains of aliases.
Tue, Dec 11, 12:01 PM
tejohnson added a comment to D53890: [LTO] Record whether LTOUnit splitting is enabled in index.

ping on this and related patches

Tue, Dec 11, 10:31 AM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.

ping

Tue, Dec 11, 10:30 AM

Thu, Dec 6

tejohnson added inline comments to D43521: [ThinLTO] Compute synthetic function entry count.
Thu, Dec 6, 1:19 PM

Wed, Dec 5

tejohnson accepted D55309: ThinLTO: Do not import debug info for imported global constants.

LGTM to address regression, especially since the debug info already isn't maintained for global variables that are const propagated. David, as a follow on it would be good to understand if/when there are cases where the debug info should be imported for these variables (i.e. if they aren't in fact constant propagated after internalization).

Wed, Dec 5, 11:37 AM
tejohnson added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.

From what I'm seeing, I would imagine this may regress debug info quality a bit. In the narrow example given, the DWARF still ends up with a description of 'foo', but without any location/value - though this is the same as if 'foo' was in the same translation unit (looks like LLVM fails to track the removing of the global and replacing everything with a constant - I believe the debug info metadata supports describing such a situation, but it isn't being used here - with or without this new change). SO that's not a regression.

But my concern would be if the global variable was in a different static library - then the user could have the variable imported, internalized, the debug info dropped there (with the patch I'm proposing), but then the original object in that static library may never be linked in (since its symbol is no longer needed) - so the resulting program has no DWARF that describes the variable.

Wed, Dec 5, 11:16 AM
tejohnson added a comment to D55309: ThinLTO: Do not import debug info for imported global constants.
Wed, Dec 5, 11:13 AM

Tue, Dec 4

tejohnson accepted D55237: LTO: Don't internalize available_externally globals..

Lgtm

Tue, Dec 4, 4:05 PM
tejohnson added inline comments to D54175: [PGO] context sensitive PGO.
Tue, Dec 4, 6:49 AM

Mon, Dec 3

tejohnson added a comment to D54175: [PGO] context sensitive PGO.
In D54175#1317243, @xur wrote:

As for the tests, I meant to add test cases to this patch later, not through a different patch.

Mon, Dec 3, 1:07 PM
tejohnson added inline comments to D54176: [PGO] clang part of change for context-sensitive PGO..
Mon, Dec 3, 11:50 AM
tejohnson added a comment to D54175: [PGO] context sensitive PGO.
In D54175#1317088, @xur wrote:

Integrated with Teresa's review comments.

I will add tests in a later patch.

Mon, Dec 3, 11:45 AM
tejohnson accepted D55060: [ThinLTO] Look through aliases in cache key calculations.

LGTM

Mon, Dec 3, 11:34 AM
tejohnson added inline comments to D54175: [PGO] context sensitive PGO.
Mon, Dec 3, 10:07 AM

Fri, Nov 30

tejohnson committed rL348068: [ThinLTO] Allow importing of functions with var args.
[ThinLTO] Allow importing of functions with var args
Fri, Nov 30, 9:14 PM
tejohnson closed D54607: [ThinLTO] Allow importing of functions with var args.
Fri, Nov 30, 9:14 PM
tejohnson created D55153: [ThinLTO] Implement index-based WPD.
Fri, Nov 30, 4:25 PM
tejohnson added a child revision for D54815: [ThinLTO] Add summary entries for index-based WPD: D55153: [ThinLTO] Implement index-based WPD.
Fri, Nov 30, 4:25 PM
tejohnson updated the diff for D54815: [ThinLTO] Add summary entries for index-based WPD.

Enable promotion of type ids in the non-split ThinLTO case and test it.

Fri, Nov 30, 4:25 PM
tejohnson updated the diff for D53890: [LTO] Record whether LTOUnit splitting is enabled in index.

Add an option to opt so that we can use the ThinLTOBitcodeWriter for
non-split ThinLTO bitcode, to enable testing the same path used when
compiling ThinLTO code via clang.

Fri, Nov 30, 4:21 PM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

I didn't report it as I was told that regular (non-thin) LTO + weak symbols didn't work. I defaulted to ThinLTO, which was working but I suppose the bug was there and just hidden like you mentioned.

Fri, Nov 30, 11:12 AM
tejohnson added inline comments to D55060: [ThinLTO] Look through aliases in cache key calculations.
Fri, Nov 30, 10:53 AM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:

http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html

Was this ever reported to binutils/gold?

Fri, Nov 30, 9:10 AM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
softirq.o: early_irq_init: PREVAILING_DEF_REG
irqdesc.o: early_irq_init: PREEMPTED_IR

Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.

Fri, Nov 30, 9:06 AM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?

Fri, Nov 30, 9:03 AM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.

The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.

Fri, Nov 30, 8:14 AM
tejohnson added a comment to D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending.

To replicate the failure, do this:

$ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
$ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
$ nm test-object.o  | grep W
0000000000001370 W arch_dynirq_lower_bound
00000000000001b0 W arch_early_irq_init
00000000000001a0 W arch_probe_nr_irqs
0000000000000190 W early_irq_init

Note that early_irq_init is still weak. It should have resolved to a concrete function.

Fri, Nov 30, 7:30 AM

Thu, Nov 29

tejohnson committed rL347892: Add missing REQUIRES to new test.
Add missing REQUIRES to new test
Thu, Nov 29, 10:05 AM
tejohnson committed rC347892: Add missing REQUIRES to new test.
Add missing REQUIRES to new test
Thu, Nov 29, 10:05 AM
tejohnson committed rL347887: [ThinLTO] Allow importing of multiple symbols with same GUID.
[ThinLTO] Allow importing of multiple symbols with same GUID
Thu, Nov 29, 9:06 AM
tejohnson committed rC347887: [ThinLTO] Allow importing of multiple symbols with same GUID.
[ThinLTO] Allow importing of multiple symbols with same GUID
Thu, Nov 29, 9:06 AM
tejohnson closed D55048: [ThinLTO] Allow importing of multiple symbols with same GUID.
Thu, Nov 29, 9:05 AM
tejohnson committed rL347886: [ThinLTO] Import local variables from the same module as caller.
[ThinLTO] Import local variables from the same module as caller
Thu, Nov 29, 9:05 AM
tejohnson closed D55047: [ThinLTO] Import local variables from the same module as caller.
Thu, Nov 29, 9:05 AM
tejohnson updated the diff for D55047: [ThinLTO] Import local variables from the same module as caller.

Address comment

Thu, Nov 29, 6:24 AM
tejohnson added inline comments to D55047: [ThinLTO] Import local variables from the same module as caller.
Thu, Nov 29, 6:21 AM

Wed, Nov 28

tejohnson accepted D55046: Linker: Add support for GlobalIFunc..

LGTM with a few minor comments.

Wed, Nov 28, 10:39 PM
tejohnson added a child revision for D55047: [ThinLTO] Import local variables from the same module as caller: D55048: [ThinLTO] Allow importing of multiple symbols with same GUID.
Wed, Nov 28, 10:22 PM
tejohnson created D55048: [ThinLTO] Allow importing of multiple symbols with same GUID.
Wed, Nov 28, 10:22 PM
tejohnson created D55047: [ThinLTO] Import local variables from the same module as caller.
Wed, Nov 28, 10:19 PM
tejohnson added inline comments to D43521: [ThinLTO] Compute synthetic function entry count.
Wed, Nov 28, 7:00 AM
tejohnson accepted D54928: [ThinLTO] Correct linkonce_any function import linkage. NFC..

lgtm

Wed, Nov 28, 6:46 AM

Mon, Nov 26

tejohnson added a comment to D54607: [ThinLTO] Allow importing of functions with var args.

ping

Mon, Nov 26, 12:48 PM
tejohnson added a comment to D54507: [ThinLTO] Handle chains of aliases.

ping

Mon, Nov 26, 12:48 PM
tejohnson committed rL347592: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.
[ThinLTO] Consolidate cache key computation between new/old LTO APIs
Mon, Nov 26, 12:44 PM
tejohnson closed D54635: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.
Mon, Nov 26, 12:44 PM

Thu, Nov 22

tejohnson accepted D54754: [ThinLTO] Assembly representation of ReadOnly attribute.

lgtm

Thu, Nov 22, 8:08 AM

Wed, Nov 21

tejohnson added inline comments to D54754: [ThinLTO] Assembly representation of ReadOnly attribute.
Wed, Nov 21, 4:34 PM
tejohnson created D54815: [ThinLTO] Add summary entries for index-based WPD.
Wed, Nov 21, 2:54 PM
tejohnson added a child revision for D53890: [LTO] Record whether LTOUnit splitting is enabled in index: D54815: [ThinLTO] Add summary entries for index-based WPD.
Wed, Nov 21, 2:54 PM

Tue, Nov 20

tejohnson abandoned D53524: [ThinLTO] Enable LTOUnit only when it is needed.

Abandoned in favor of new approach in D53890/D53891.

Tue, Nov 20, 2:05 PM
tejohnson retitled D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed from [LTO] Pass down LTOUnit codegen flag to bitcode writer to [LTO] Add option to enable LTOUnit splitting, and disable unless needed.
Tue, Nov 20, 2:04 PM
tejohnson retitled D53890: [LTO] Record whether LTOUnit splitting is enabled in index from [LTO] Record LTOUnit flag in index and validate during LTO link to [LTO] Record whether LTOUnit splitting is enabled in index.
Tue, Nov 20, 2:03 PM
tejohnson updated the diff for D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed.

Update to use new module flag

Tue, Nov 20, 1:52 PM
tejohnson updated the diff for D53890: [LTO] Record whether LTOUnit splitting is enabled in index.

Changed to use a new module flag to represent whether splitting enabled.
This has a couple advantages:

  1. Cleaner - doesn't require threading the flag through various interfaces.
  2. For my follow on patch to add new summary info for index-based WPD,

need to know whether splitting enabled during buildModuleSummaryIndex,
and it is not clear how to pass down a flag when this is invoked via
ModuleSummaryIndexAnalysis::run* invoked by getResult<>/getAnalysis<>
from the BitcodeWriterPass. Adding the flag to the index after the index
is built (previous version) is too late.

Tue, Nov 20, 1:51 PM

Sat, Nov 17

tejohnson committed rL347146: Fix bot failure from r347145.
Fix bot failure from r347145
Sat, Nov 17, 12:44 PM
tejohnson committed rL347145: [ThinLTO] Add some stats for read only variable internalization.
[ThinLTO] Add some stats for read only variable internalization
Sat, Nov 17, 12:06 PM
tejohnson closed D54642: [ThinLTO] Add some stats for read only variable internalization.
Sat, Nov 17, 12:06 PM

Fri, Nov 16

tejohnson added a comment to D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed.
  • Switch the default of splitting lto units to off by default, unless compiled with CFI or -fwhole-program-vtables.
Fri, Nov 16, 3:25 PM
tejohnson added a comment to D54635: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.

I wonder freestanding is still an issue after r316819. There is a function attribute for that now so it should be respected if the module is built with -ffreestanding and it should create different hash. I will do some digging to see if I can find the original report and how they setup the build. @mehdi_amini, do you remember the original problem?

Fri, Nov 16, 1:17 PM
tejohnson created D54642: [ThinLTO] Add some stats for read only variable internalization.
Fri, Nov 16, 11:32 AM
tejohnson created D54635: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.
Fri, Nov 16, 10:00 AM

Thu, Nov 15

tejohnson updated the diff for D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed.

Update a couple tests due to new default of splitting off and new flag.

Thu, Nov 15, 10:09 PM
tejohnson updated the diff for D53890: [LTO] Record whether LTOUnit splitting is enabled in index.

Update a couple tests due to new default of splitting off and new flag.

Thu, Nov 15, 10:09 PM
tejohnson updated the diff for D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed.
  • As discussed off-patch, will use a new, separate option to control this

splitting, here I am using -f[no]split-lto-unit.

  • Switch the default of splitting lto units to off by default, unless

compiled with CFI or -fwhole-program-vtables.

  • I'll update the patch title and summary once we converge on flag/behavior.
Thu, Nov 15, 9:46 PM
tejohnson updated the diff for D53890: [LTO] Record whether LTOUnit splitting is enabled in index.
  • Switch to an EnableSplitLTOUnit flag.
  • Record the value of !EnableSplitLTOUnit in the bitcode for compatibility with current code that has splitting enabled by default
  • As discussed off-patch, will use a new, separate option to control this

splitting, here I am using -f[no]split-lto-unit.

  • I'll update the patch title and summary once we converge on flag/behavior.
Thu, Nov 15, 9:34 PM
tejohnson created D54607: [ThinLTO] Allow importing of functions with var args.
Thu, Nov 15, 4:36 PM
tejohnson added a comment to D49362: [ThinLTO] Internalize read only globals.

I was trying this out internally and noticed a couple of minor things that can be fixed when you recommit the patch with the caching fix.

Thu, Nov 15, 4:28 PM
tejohnson accepted D54564: [LTO] Load sample profile in LTO link step..

LGTM

Thu, Nov 15, 10:07 AM
tejohnson added inline comments to D54564: [LTO] Load sample profile in LTO link step..
Thu, Nov 15, 9:53 AM
tejohnson added inline comments to D54564: [LTO] Load sample profile in LTO link step..
Thu, Nov 15, 9:34 AM
tejohnson added a comment to D54564: [LTO] Load sample profile in LTO link step..

Looks like the same issue exists in the new pass manager. The sample loader is added via buildModuleSimplificationPipeline, which is not called either directly or indirectly from buildLTODefaultPipeline. Can you add and test the fix there too? Thanks!

Thu, Nov 15, 5:58 AM

Nov 14 2018

tejohnson committed rL346899: Remove unused getMDNodeFwdRefOrNull interfaces (NFC).
Remove unused getMDNodeFwdRefOrNull interfaces (NFC)
Nov 14 2018, 2:00 PM
tejohnson closed D54542: Remove unused getMDNodeFwdRefOrNull interfaces (NFC).
Nov 14 2018, 2:00 PM
tejohnson added a comment to D54542: Remove unused getMDNodeFwdRefOrNull interfaces (NFC).

@steven_wu I realized there are still some uses of the internal getMDNodeFwdRefOrNull implementation, so just removed the external interfaces. It seemed like switching to returning ErrorOr<MDNode*> actually made things more complicated, so I just tried to improve the existing error messages. Let me know if you have something else in mind.

Nov 14 2018, 1:54 PM
tejohnson created D54542: Remove unused getMDNodeFwdRefOrNull interfaces (NFC).
Nov 14 2018, 1:53 PM
tejohnson closed D53596: [ThinLTO] Fix a crash in lazy loading of Metadata.

My commit message lost the phabricator link, so this didn't get auto-closed. Closed by commit r346891.

Nov 14 2018, 1:09 PM