Rafael Espíndola
2014-Aug-28 04:13 UTC
[LLVMdev] How to tell whether a GlobalValue is user-defined
>> Agreed. If ld64 can drop support for .o produced by the old gcc that >> would be awesome. Failing that, what is really needed is > Because of static archives, the linker has to support old .o files for quite a while. I also don’t know when clang starting getting this right.r123585 (Jan 16 17:19:34 2011) I think.> Also, this seems like linker complexity for a very unlikely optimization (two named constants with same value). If someone cares about this level of optimization, they can use LTO which will do all the constant merging in LLVM. > >> >> LLVM should only put constants in mergeable sections only if (among >> other things) they require only symbols that start with 'l' or 'L'. > Not sure what you mean here. What is "requiring”? Are we talking about this code in TargetLoweringObjectFileMachO::SelectSectionForGlobal()I mean "the correspoinding symbol name will start with".> if (Kind.isMergeableConst()) { > if (Kind.isMergeableConst4()) > return FourByteConstantSection; > if (Kind.isMergeableConst8()) > return EightByteConstantSection; > if (Kind.isMergeableConst16()) > return SixteenByteConstantSection; > } > > Can’t we just change the first ‘if’ to: > > if (Kind.isMergeableConst() && !GV.hasName()) { > > That should leave any “named” constants in the __const section instead of moving them to the __literal section. (Though I don’t actually know if anonymous constants were given some name earlier so hasName() is useless at this point).That seems too strict. A private GV can have a name, but it will be printed with a 'L' or 'l' prefix, so it should not be a problem. In other words, it looks like you want something like the attached patch. Cheers, Rafael -------------- next part -------------- A non-text attachment was scrubbed... Name: t.patch Type: text/x-patch Size: 2241 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140828/7f63664d/attachment.bin>
David Majnemer
2014-Aug-28 04:32 UTC
[LLVMdev] How to tell whether a GlobalValue is user-defined
On Wed, Aug 27, 2014 at 9:13 PM, Rafael Espíndola < rafael.espindola at gmail.com> wrote:> >> Agreed. If ld64 can drop support for .o produced by the old gcc that > >> would be awesome. Failing that, what is really needed is > > Because of static archives, the linker has to support old .o files for > quite a while. I also don’t know when clang starting getting this right. > > r123585 (Jan 16 17:19:34 2011) I think. > > > Also, this seems like linker complexity for a very unlikely optimization > (two named constants with same value). If someone cares about this level > of optimization, they can use LTO which will do all the constant merging in > LLVM. > > > >> > >> LLVM should only put constants in mergeable sections only if (among > >> other things) they require only symbols that start with 'l' or 'L'. > > Not sure what you mean here. What is "requiring”? Are we talking about > this code in TargetLoweringObjectFileMachO::SelectSectionForGlobal() > > I mean "the correspoinding symbol name will start with". > > > if (Kind.isMergeableConst()) { > > if (Kind.isMergeableConst4()) > > return FourByteConstantSection; > > if (Kind.isMergeableConst8()) > > return EightByteConstantSection; > > if (Kind.isMergeableConst16()) > > return SixteenByteConstantSection; > > } > > > > Can’t we just change the first ‘if’ to: > > > > if (Kind.isMergeableConst() && !GV.hasName()) { > > > > That should leave any “named” constants in the __const section instead > of moving them to the __literal section. (Though I don’t actually know if > anonymous constants were given some name earlier so hasName() is useless at > this point). > > That seems too strict. A private GV can have a name, but it will be > printed with a 'L' or 'l' prefix, so it should not be a problem. > > In other words, it looks like you want something like the attached patch. >Rafael, would it be more correct to use hasLocalLinkage() instead of hasPrivateLinkage() ?> > Cheers, > Rafael > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140827/7545274b/attachment.html>
Rafael Espíndola
2014-Aug-28 14:49 UTC
[LLVMdev] How to tell whether a GlobalValue is user-defined
> Rafael, would it be more correct to use hasLocalLinkage() instead of > hasPrivateLinkage() ?No, hasLocalLinkage covers InternalLinkage too. The difference from private to internal is that internal shows up in the symbol table. For private llvm mangles the name to add a L or l prefix which causes the assembler (or linker) to omit it Cheers, Rafael
Akira Hatanaka
2014-Aug-28 19:26 UTC
[LLVMdev] How to tell whether a GlobalValue is user-defined
I noticed that some of the passes are creating GlobalVariables that should be merged using InternalLinkage instead of PrivateLinkage. For example, global array ".memset_pattern" created by LoopIdiomRecognize has InternalLinkage. Since these constants won't be prefixed with "L" or "l", will these constants be placed in section __const too? Your patch isn't preventing any constants that could be merged before from being merged, but it just seems to me that those constants should go into the mergeable sections too, like "switch.table" does. Otherwise, the patch looks fine to me. On Wed, Aug 27, 2014 at 9:13 PM, Rafael Espíndola < rafael.espindola at gmail.com> wrote:> >> Agreed. If ld64 can drop support for .o produced by the old gcc that > >> would be awesome. Failing that, what is really needed is > > Because of static archives, the linker has to support old .o files for > quite a while. I also don’t know when clang starting getting this right. > > r123585 (Jan 16 17:19:34 2011) I think. > > > Also, this seems like linker complexity for a very unlikely optimization > (two named constants with same value). If someone cares about this level > of optimization, they can use LTO which will do all the constant merging in > LLVM. > > > >> > >> LLVM should only put constants in mergeable sections only if (among > >> other things) they require only symbols that start with 'l' or 'L'. > > Not sure what you mean here. What is "requiring”? Are we talking about > this code in TargetLoweringObjectFileMachO::SelectSectionForGlobal() > > I mean "the correspoinding symbol name will start with". > > > if (Kind.isMergeableConst()) { > > if (Kind.isMergeableConst4()) > > return FourByteConstantSection; > > if (Kind.isMergeableConst8()) > > return EightByteConstantSection; > > if (Kind.isMergeableConst16()) > > return SixteenByteConstantSection; > > } > > > > Can’t we just change the first ‘if’ to: > > > > if (Kind.isMergeableConst() && !GV.hasName()) { > > > > That should leave any “named” constants in the __const section instead > of moving them to the __literal section. (Though I don’t actually know if > anonymous constants were given some name earlier so hasName() is useless at > this point). > > That seems too strict. A private GV can have a name, but it will be > printed with a 'L' or 'l' prefix, so it should not be a problem. > > In other words, it looks like you want something like the attached patch. > > Cheers, > Rafael >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140828/2acbf739/attachment.html>
Rafael Espíndola
2014-Aug-28 20:26 UTC
[LLVMdev] How to tell whether a GlobalValue is user-defined
On 28 August 2014 15:26, Akira Hatanaka <ahatanak at gmail.com> wrote:> I noticed that some of the passes are creating GlobalVariables that should > be merged using InternalLinkage instead of PrivateLinkage. For example, > global array ".memset_pattern" created by LoopIdiomRecognize has > InternalLinkage. Since these constants won't be prefixed with "L" or "l", > will these constants be placed in section __const too?Correct.> Your patch isn't > preventing any constants that could be merged before from being merged, but > it just seems to me that those constants should go into the mergeable > sections too, like "switch.table" does.They should use private linkage then. It seems an unavoidable consequence of the ld64 restriction. Since it requires mergeable constants to start with 'l' or 'L', we have to use a linkage that allows LLVM to remove the symbol from the symbol table. That linkage is private.> Otherwise, the patch looks fine to me.r216682. Cheers, Rafael