[PATCH] Substantial RuntimeDyldMachO cleanup.
Philip Reames
listmail at philipreames.com
Fri Jun 20 16:36:37 PDT 2014
The overall direction seems quite reasonable.
I haven't looked at the patch in any detail under the assumption you're
mostly just moving code around. A couple small comments:
- Do all these functions need to be defined in headers?
- Do the functions on RuntimeDyldMach0 need to be marked virtual or
override?
- Can we move the DEBUG_TYPE macros out of the headers and into source
files?
Philip
On 06/20/2014 01:36 PM, Lang Hames wrote:
> Ping?
>
> I know Andy is out on vacation, but do any other MCJIT people have any
> thoughts on this?
>
> Cheers,
> Lang.
>
>
>
> On Thu, Jun 12, 2014 at 1:34 PM, Lang Hames <lhames at gmail.com
> <mailto:lhames at gmail.com>> wrote:
>
> Hi All,
>
> 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.
>
> 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:
> (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.
> (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.
>
> To fix these issues, this patch splits RuntimeDyldMachO's
> functionality across the following class hierarchy:
>
> RuntimeDyldMachO
> Implemented in RuntimeDyldMachO.{h,cpp}
> 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.
>
> template <typename Impl>
> RuntimeDyldMachOCRTPBase<Impl> : public RuntimeDyldMachO
> Implemented in RuntimeDyldMachO.h
> 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.
>
> RuntimeDyldMachOARM : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM>
> RuntimeDyldMachOARM64 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM64>
> RuntimeDyldMachOI386 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOI386>
> RuntimeDyldMachOX86_64 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOX86_64>
> Implemented in their respective *.h files in
> lib/ExecutionEngine/RuntimeDyld/MachOTargets
> Each of these contains the relocation logic specific to their
> target architecture.
>
> 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.
>
> Please let me know what you think.
>
> Cheers,
> Lang.
>
>
> [1] http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140620/516a895d/attachment.html>
More information about the llvm-commits
mailing list