<div dir="ltr">If MCSymbol ever ends up with more-than-pointer alignment you'd need to add padding between the pointer and the MCSymbol - maybe best just to assert that MCSymbol's alignment <= pointer alignment (& then just use alignof(NameTy*))?<br><br>"getNamePtr" maybe rename to "getNameEntryPtr" and have getNamePtr functions return NameTy*& rather than NameTy**? (usual prefer reference over pointer)<br><br>& maybe NameTy could be NameEntryTy - NameTy is a bit confusing when it's not the name mbut a think you can access the name from.<br><br>(though perhaps these names have precedence from the types you modeled them off, I'm not sure)<br><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 2:06 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Attached an updated patch with all of the feedback from David fixed.<div><br></div><div>Cheers,</div><div>Pete</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><div><blockquote type="cite"><div>On Jun 8, 2015, at 2:02 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:</div><br><div><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br>On 2015 Jun 8, at 13:50, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><br><br><br>On Mon, Jun 8, 2015 at 1:45 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br><br><blockquote type="cite">On 2015 Jun 8, at 13:16, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>> wrote:<br><br><br><blockquote type="cite">On Jun 8, 2015, at 1:11 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><br><br><br>On Mon, Jun 8, 2015 at 12:19 PM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>> wrote:<br>Hi all<br><br>Recent work by Rafael and Duncan has greatly reduced the number of MCSymbol’s which are actually named.  In particular, temporary symbols typically don’t need names.  On the verify_uselist_order test case for example, only around 50,000 symbols have names out of 700,000 total symbols.<br><br>This patch moves the storage for the Name* itself to be allocated prior to the MCSymbol.<br><br>Any particular reason it goes before rather than after? (I know some of our data structures put things after)<br></blockquote>There’s a couple of subclasses of MCSymbol - MCSymbolCOFF and MCSymbolELF - so i thought it would be tricker to put it after those.<br><blockquote type="cite"><br>There's no need to reinterpret_cast to MCSymbol when you just want a void* in operator new.<br></blockquote>Good catch.  Thanks.  I’ll fix that.<br></blockquote><br>FWIW, I've found the opposite in a few places in our code base.<br>Sometimes you need to cast to `void *` to disambiguate from other<br>versions of `operator new()`.<br><br>Not quite sure I follow - In other contexts it may be relevant, but in that particular context (casting T* to U* just to return the U* as void* anyway) it doesn't seem to add anything.<br></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">Sure, I didn't actually look at the code :).</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br><br><blockquote type="cite"><blockquote type="cite"><br>The alignment of the allocation might be interesting - given that it needs to be aligned as much as a pointer, not just MCSymbol (I assume MCSymbol is at least as aligned as a pointer right now, so you probably get this for free/correct without explicitly accounting for it)<br></blockquote>Thats true.  There are currently pointers in there, but good to be explicit about it.  I’ll fix that too.<br><blockquote type="cite"><br>The extra indirection hidden behind NameTy is a bit confusing (especially since you have pointers to it - so it's pointers on pointers).<br></blockquote>Yeah, I see what you mean.  I’ll move the * out of NameTy.<br><br>Thanks for the comments.  I’ll send out an updated patch shortly.<br><br>Cheers,<br>Pete<br><blockquote type="cite"><br>This is similar to how User allocates Use’s in a custom new/delete.<br><br>Given that a symbol is currently 48 bytes on MachO, this reduces it by 16% on the typical case.  I have other patches coming which will get us to 24-bytes which is on average half the current amount.<br><br>Cheers,<br>Pete<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></blockquote></blockquote></div></blockquote></div><br></div></div><br></blockquote></div><br></div>