[PATCH] Substantial RuntimeDyldMachO cleanup.
Philip Reames
listmail at philipreames.com
Mon Jun 23 17:09:56 PDT 2014
Lang,
I'm happy with all of your answers and like the general direction. If
others approve, go for it.
Philip
On 06/23/2014 04:47 PM, Lang Hames wrote:
> CCing Nick Kledzik as suggested by Rafael.
>
> - Lang.
>
>
> On Mon, Jun 23, 2014 at 3:49 PM, Lang Hames <lhames at gmail.com
> <mailto:lhames at gmail.com>> wrote:
>
> Hi Philip,
>
> Thanks for taking a look!
>
> Answering point-by-point:
>
> (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.
>
> (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.
>
> (3) I can remove the DEBUG_TYPE defines from the target-specific
> files if I push the include order around in RuntimeDyldMachO.h:
>
> From what we have now (with the DEBUG_TYPE defined in the headers):
>
> #include ...
> #include "MachOTargets/..."
> #include "MachOTargets/..."
> #include ...
>
> To:
>
> #include ...
> #include ...
> // End of non-target includes.
>
> #define DEBUG_TYPE "dyld"
> #include "MachOTargets/..."
> #include "MachOTargets/..."
> // End target includes.
>
> 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.
>
> Cheers,
> Lang.
>
>
>
> On Fri, Jun 20, 2014 at 4:36 PM, Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
> 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 <mailto: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/20140623/3bb15402/attachment.html>
More information about the llvm-commits
mailing list