<div dir="ltr">CCing Nick Kledzik as suggested by Rafael.<div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 23, 2014 at 3:49 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Philip,<div><br></div><div>Thanks for taking a look!</div><div><br></div><div>Answering point-by-point:</div>
<div><br></div><div>(1) These functions don't have to be defined in the headers, but that's probably where we want them. Part of the intent of the CRTP pattern is these definitions should be inlined into the parent class (RuntimeDyldCRTPBase), so they need to be present when the instantiations of the parent class are used (in RuntimeDyldMachO.cpp). That means either putting the code for all MachO targets in to RuntimeDyldMachO.cpp, or having one header per target. I think the latter is cleaner.</div>
<div><br></div><div>(2) Yes for now, but my plan is to start removing some of the dynamism from the RuntimeDyldImpl class tree in the future. That work was omitted from this patch as it was already getting large, and I wanted to clear the general direction with the community first.</div>
<div><br></div><div>(3) I can remove the DEBUG_TYPE defines from the target-specific files if I push the include order around in RuntimeDyldMachO.h:</div><div><br></div><div>From what we have now (with the DEBUG_TYPE defined in the headers):</div>
<div><br></div><div>#include ...</div><div>#include "MachOTargets/..."</div><div>#include "MachOTargets/..."</div><div>#include ...</div><div><br></div><div>To:</div><div><br></div><div><div>#include ...</div>
</div><div><div>#include ...</div></div><div>// End of non-target includes.</div><div><br></div><div>#define DEBUG_TYPE "dyld"</div><div><div>#include "MachOTargets/..."</div></div><div><div>#include "MachOTargets/..."</div>
</div><div>// End target includes.</div><div><br></div><div>The effect is the same either way: We can use the DEBUG macro from the target specific headers. I think I actually prefer the latter, and I'd be happy to switch to it. Neither is canonical, but that's down to the weirdness of the CRTP.</div>
<div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 20, 2014 at 4:36 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
The overall direction seems quite reasonable. <br>
<br>
I haven't looked at the patch in any detail under the assumption
you're mostly just moving code around. A couple small comments:<br>
- Do all these functions need to be defined in headers?<br>
- Do the functions on RuntimeDyldMach0 need to be marked virtual or
override?<br>
- Can we move the DEBUG_TYPE macros out of the headers and into
source files?<br>
<br>
Philip<div><div><br>
<br>
<br>
<br>
<div>On 06/20/2014 01:36 PM, Lang Hames
wrote:<br>
</div>
</div></div><blockquote type="cite"><div><div>
<div dir="ltr">Ping?
<div><br>
</div>
<div>I know Andy is out on vacation, but do any other MCJIT
people have any thoughts on this?</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Lang.</div>
<div><br>
</div>
</div>
<div class="gmail_extra">
<br>
<br>
<div class="gmail_quote">On Thu, Jun 12, 2014 at 1:34 PM, Lang
Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Hi All,
<div><br>
</div>
<div>Attached is a patch that I think substantially cleans
up RuntimeDyldMachO and, if the general direction is
agreed with, could be used as a template for cleaning up
RuntimeDyldELF.</div>
<div><br>
</div>
<div>At the moment all MCJIT relocation handling logic for
MachO is contained in the RuntimeDyldMachO class (see
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.{h,cpp}).
Mixing the logic for all different MachO targets in a
single class creates two problems:</div>
<div>(1) Poor readability/maintainability: Any change to
the relocation logic for a particular target has to be
inspected carefully to make sure it won't alter the
behavior for any other target. Some of my recent patches
for X86 have exacerbated this problem, so I'm very happy
to be able to offer this patch as penance.</div>
<div>(2) Poor performance: we're querying the target
architecture in multiple places for every relocation,
despite it being a known quantity as soon as we load an
object file.</div>
<div><br>
</div>
<div>To fix these issues, this patch splits
RuntimeDyldMachO's functionality across the following
class hierarchy:</div>
<div><br>
</div>
<div><font face="courier new, monospace">RuntimeDyldMachO</font></div>
<div>Implemented in RuntimeDyldMachO.{h,cpp}</div>
<div>Contains logic that is completely independent of the
target. This consists mostly of MachO helper utilities
which the derived classes use to get their work done.</div>
<div><br>
</div>
<div>t<font face="courier new, monospace">emplate
<typename Impl></font></div>
<div><font face="courier new, monospace">RuntimeDyldMachOCRTPBase<Impl>
: public RuntimeDyldMachO</font></div>
<div>
Implemented in RuntimeDyldMachO.h</div>
<div>Contains generic MachO algorithms/data structures
that defer to the Impl class for target-specific
behaviors. For the curious, and as the name suggests,
this acts as the base of a CRTP[1] instance.</div>
<div><br>
</div>
<div><font face="courier new, monospace">RuntimeDyldMachOARM
: public
RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM></font></div>
<div><font face="courier new, monospace">RuntimeDyldMachOARM64
: public
RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM64></font></div>
<div><font face="courier new, monospace">RuntimeDyldMachOI386
: public
RuntimeDyldMachOCRTPBase<RuntimeDyldMachOI386></font></div>
<div><font face="courier new, monospace">RuntimeDyldMachOX86_64
: public
RuntimeDyldMachOCRTPBase<RuntimeDyldMachOX86_64></font></div>
<div>Implemented in their respective *.h files in
lib/ExecutionEngine/RuntimeDyld/MachOTargets</div>
<div>Each of these contains the relocation logic specific
to their target architecture.</div>
<div><br>
</div>
<div>These target specific classes keep the target
specific craziness mercifully well contained, compared
to what we have today. Each target can be modified
independently, without concern for breaking other
targets. The 'switch (Arch) ...' anti-pattern is gone,
presumably with a small performance benefit. There's
still lots of work to do, but I think this is a solid
first step.</div>
<div><br>
</div>
<div>Please let me know what you think.</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Lang.</div>
<div><br>
</div>
<div><br>
</div>
<div>[1] <a href="http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern" target="_blank">http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern</a></div>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<fieldset></fieldset>
<br>
</div></div><pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>