<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body 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<br>
<br>
<br>
<br>
<div class="moz-cite-prefix">On 06/20/2014 01:36 PM, Lang Hames
wrote:<br>
</div>
<blockquote
cite="mid:CALLttgr5mS=hmwnNcXifgPjsj5d0SQkjGdS1qV20U3VpoONM8w@mail.gmail.com"
type="cite">
<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 class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>