<div dir="ltr">Hi Eric, Phil,<div><br></div><div>Sorry about the delayed reply - I was out on holidays for a bit.</div><div><br></div><div>Attached is an updated patch:</div><div>- The ARM64 class has been renamed to AArch64.</div>
<div>- I've fixed the weirdly named 'do' methods.</div><div>- Everything has been run through clang format.</div><div><br></div><div>Thanks very much for the review and feedback. If this all looks good to you I'll go ahead and commit.</div>
<div><br></div><div>Cheers,</div><div>Lang.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 24, 2014 at 11:24 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">One other small nit:<br>
<br>
The naming etc of process and do is really weird. Renaming those for<br>
clarity (or great justice, your choice) might be nice.<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, Jun 24, 2014 at 11:24 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
> Seems good to me as well.<br>
><br>
> One small nit: Might be reasonable to migrate the ARM64 classes to<br>
> AArch64 to match the rest of llvm? (The relocation names, of course,<br>
> will have to stay the same.)<br>
><br>
> -eric<br>
><br>
> On Mon, Jun 23, 2014 at 5:09 PM, Philip Reames<br>
> <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br>
>> Lang,<br>
>><br>
>> I'm happy with all of your answers and like the general direction.  If<br>
>> others approve, go for it.<br>
>><br>
>> Philip<br>
>><br>
>><br>
>> On 06/23/2014 04:47 PM, Lang Hames wrote:<br>
>><br>
>> CCing Nick Kledzik as suggested by Rafael.<br>
>><br>
>> - Lang.<br>
>><br>
>><br>
>> On Mon, Jun 23, 2014 at 3:49 PM, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>
>>><br>
>>> Hi Philip,<br>
>>><br>
>>> Thanks for taking a look!<br>
>>><br>
>>> Answering point-by-point:<br>
>>><br>
>>> (1) These functions don't have to be defined in the headers, but that's<br>
>>> probably where we want them. Part of the intent of the CRTP pattern is these<br>
>>> definitions should be inlined into the parent class (RuntimeDyldCRTPBase),<br>
>>> so they need to be present when the instantiations of the parent class are<br>
>>> used (in RuntimeDyldMachO.cpp). That means either putting the code for all<br>
>>> MachO targets in to RuntimeDyldMachO.cpp, or having one header per target. I<br>
>>> think the latter is cleaner.<br>
>>><br>
>>> (2) Yes for now, but my plan is to start removing some of the dynamism<br>
>>> from the RuntimeDyldImpl class tree in the future. That work was omitted<br>
>>> from this patch as it was already getting large, and I wanted to clear the<br>
>>> general direction with the community first.<br>
>>><br>
>>> (3) I can remove the DEBUG_TYPE defines from the target-specific files if<br>
>>> I push the include order around in RuntimeDyldMachO.h:<br>
>>><br>
>>> From what we have now (with the DEBUG_TYPE defined in the headers):<br>
>>><br>
>>> #include ...<br>
>>> #include "MachOTargets/..."<br>
>>> #include "MachOTargets/..."<br>
>>> #include ...<br>
>>><br>
>>> To:<br>
>>><br>
>>> #include ...<br>
>>> #include ...<br>
>>> // End of non-target includes.<br>
>>><br>
>>> #define DEBUG_TYPE "dyld"<br>
>>> #include "MachOTargets/..."<br>
>>> #include "MachOTargets/..."<br>
>>> // End target includes.<br>
>>><br>
>>> The effect is the same either way: We can use the DEBUG macro from the<br>
>>> target specific headers. I think I actually prefer the latter, and I'd be<br>
>>> happy to switch to it. Neither is canonical, but that's down to the<br>
>>> weirdness of the CRTP.<br>
>>><br>
>>> Cheers,<br>
>>> Lang.<br>
>>><br>
>>><br>
>>><br>
>>> On Fri, Jun 20, 2014 at 4:36 PM, Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> The overall direction seems quite reasonable.<br>
>>>><br>
>>>> I haven't looked at the patch in any detail under the assumption you're<br>
>>>> 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<br>
>>>> override?<br>
>>>> - Can we move the DEBUG_TYPE macros out of the headers and into source<br>
>>>> files?<br>
>>>><br>
>>>> Philip<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> On 06/20/2014 01:36 PM, Lang Hames wrote:<br>
>>>><br>
>>>> Ping?<br>
>>>><br>
>>>> I know Andy is out on vacation, but do any other MCJIT people have any<br>
>>>> thoughts on this?<br>
>>>><br>
>>>> Cheers,<br>
>>>> Lang.<br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Thu, Jun 12, 2014 at 1:34 PM, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>
>>>>><br>
>>>>> Hi All,<br>
>>>>><br>
>>>>> Attached is a patch that I think substantially cleans up<br>
>>>>> RuntimeDyldMachO and, if the general direction is agreed with, could be used<br>
>>>>> as a template for cleaning up RuntimeDyldELF.<br>
>>>>><br>
>>>>> At the moment all MCJIT relocation handling logic for MachO is contained<br>
>>>>> in the RuntimeDyldMachO class (see<br>
>>>>> lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.{h,cpp}). Mixing the logic<br>
>>>>> for all different MachO targets in a single class creates two problems:<br>
>>>>> (1) Poor readability/maintainability: Any change to the relocation logic<br>
>>>>> for a particular target has to be inspected carefully to make sure it won't<br>
>>>>> alter the behavior for any other target. Some of my recent patches for X86<br>
>>>>> have exacerbated this problem, so I'm very happy to be able to offer this<br>
>>>>> patch as penance.<br>
>>>>> (2) Poor performance: we're querying the target architecture in multiple<br>
>>>>> places for every relocation, despite it being a known quantity as soon as we<br>
>>>>> load an object file.<br>
>>>>><br>
>>>>> To fix these issues, this patch splits RuntimeDyldMachO's functionality<br>
>>>>> across the following class hierarchy:<br>
>>>>><br>
>>>>> RuntimeDyldMachO<br>
>>>>> Implemented in RuntimeDyldMachO.{h,cpp}<br>
>>>>> Contains logic that is completely independent of the target. This<br>
>>>>> consists mostly of MachO helper utilities which the derived classes use to<br>
>>>>> get their work done.<br>
>>>>><br>
>>>>> template <typename Impl><br>
>>>>> RuntimeDyldMachOCRTPBase<Impl> : public RuntimeDyldMachO<br>
>>>>> Implemented in RuntimeDyldMachO.h<br>
>>>>> Contains generic MachO algorithms/data structures that defer to the Impl<br>
>>>>> class for target-specific behaviors. For the curious, and as the name<br>
>>>>> suggests, this acts as the base of a CRTP[1] instance.<br>
>>>>><br>
>>>>> RuntimeDyldMachOARM : public<br>
>>>>> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM><br>
>>>>> RuntimeDyldMachOARM64 : public<br>
>>>>> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM64><br>
>>>>> RuntimeDyldMachOI386 : public<br>
>>>>> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOI386><br>
>>>>> RuntimeDyldMachOX86_64 : public<br>
>>>>> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOX86_64><br>
>>>>> Implemented in their respective *.h files in<br>
>>>>> lib/ExecutionEngine/RuntimeDyld/MachOTargets<br>
>>>>> Each of these contains the relocation logic specific to their target<br>
>>>>> architecture.<br>
>>>>><br>
>>>>> These target specific classes keep the target specific craziness<br>
>>>>> mercifully well contained, compared to what we have today. Each target can<br>
>>>>> be modified independently, without concern for breaking other targets. The<br>
>>>>> 'switch (Arch) ...' anti-pattern is gone, presumably with a small<br>
>>>>> performance benefit. There's still lots of work to do, but I think this is a<br>
>>>>> solid first step.<br>
>>>>><br>
>>>>> Please let me know what you think.<br>
>>>>><br>
>>>>> Cheers,<br>
>>>>> Lang.<br>
>>>>><br>
>>>>><br>
>>>>> [1] <a href="http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern" target="_blank">http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern</a><br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> _______________________________________________<br>
>>>> llvm-commits mailing list<br>
>>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>>><br>
>>>><br>
>>><br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>><br>
</div></div></blockquote></div><br></div>