<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Lang,<br>
<br>
I'm happy with all of your answers and like the general direction.
If others approve, go for it.<br>
<br>
Philip<br>
<br>
<div class="moz-cite-prefix">On 06/23/2014 04:47 PM, Lang Hames
wrote:<br>
</div>
<blockquote
cite="mid:CALLttgr-1oiCXONdh4sJ-GvcXQGNXgKMMESnegGXhTRa2XKX5w@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true" href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a moz-do-not-send="true" 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>
</blockquote>
<br>
</body>
</html>