<div dir="ltr">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.<div>
<br></div><div>Otherwise, the patch looks fine to me.</div><div><br></div><div><div><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 27, 2014 at 9:13 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">>> Agreed. If ld64 can drop support for .o produced by the old gcc that<br>

>> would be awesome. Failing that, what is really needed is<br>
> 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.<br>
<br>
</div>r123585 (Jan 16 17:19:34 2011) I think.<br>
<div class=""><br>
> 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.<br>

><br>
>><br>
>> LLVM should only put constants in mergeable sections only if (among<br>
>> other things) they require only symbols that start with 'l' or 'L'.<br>
> Not sure what you mean here. What is "requiring”?  Are we talking about this code in TargetLoweringObjectFileMachO::SelectSectionForGlobal()<br>
<br>
</div>I mean "the correspoinding symbol name will start with".<br>
<div class=""><br>
>  if (Kind.isMergeableConst()) {<br>
>     if (Kind.isMergeableConst4())<br>
>       return FourByteConstantSection;<br>
>     if (Kind.isMergeableConst8())<br>
>       return EightByteConstantSection;<br>
>     if (Kind.isMergeableConst16())<br>
>       return SixteenByteConstantSection;<br>
>   }<br>
><br>
> Can’t we just change the first ‘if’ to:<br>
><br>
>  if (Kind.isMergeableConst() && !GV.hasName()) {<br>
><br>
> 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).<br>

<br>
</div>That seems too strict. A private GV can have a name, but it will be<br>
printed with a 'L' or 'l' prefix, so it should not be a problem.<br>
<br>
In other words, it looks like you want something like the attached patch.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div></div></div>