[PATCH] DataLayout is mandatory, update the API to reflect it with references.

[Eric Christopher]

On Mon, Mar 9, 2015 at 4:19 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> On Mar 9, 2015, at 3:03 PM, Eric Christopher <echristo at gmail.com> wrote:
>
>
>
> On Mon, Mar 9, 2015 at 2:49 PM Mehdi AMINI <mehdi.amini at apple.com> wrote:
>
>> Thanks for review, that was quick :)
>>
>>
>>
> Sure :)
>
>
>> REPOSITORY
>>   rL LLVM
>>
>> ================
>> Comment at: lib/Analysis/Lint.cpp:178
>> @@ -177,3 +176,2 @@
>>    DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
>> -  DL = &F.getParent()->getDataLayout();
>>    TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
>> ----------------
>> echristo wrote:
>> > Any reason not to leave it cached instead of passing it down?
>> In short: cannot rebind a reference and I wanted to get rid of the
>> pointer.
>>
>>
> Ah, sure.
>
>
>> It plays well with passes that initialize a separate transient object to
>> do the processing, but not with passes that do their processing "in-place".
>> Because passes object are not transient, they carry their state between
>> runs, including a potential obsolete DL pointer.
>>
>
>
>> Not sure if I was clear?
>>
>>
> Assuming in the case of "things that work on multiple modules". Otherwise,
> something that just runs on a single module should be able to cache it with
> no problem right? :)
>
>
> Even in the case “work on multiple modules” it should be fine. The problem
> is to be sure that all the paths that can call a function that used the
> cached DL are coming from the entry point of the pass execution
> (runOnFunction() and cie), which is supposed to update the cached pointer.
>

Sure.


> This makes the code a bit harder to analyze compared to transient object.
> Note that on the opposite, I added a cached DataLayout sometimes where it
> was possible in order to avoid passing an extra argument (for
> instance SCEVExpander).
>
> I have a cleanup update coming soon.
>
>
Cool deal. Thanks!

-eric


>  —
> Mehdi
>
>
>
>
>
>> This was very case-by-case, and I may have get it wrong here and there (I
>> mean wrong in the sense that a function argument could have been save by
>> retrieving the DL differently, I don't think I broke correctness).
>>
>>
> Right. Should be ok.
>
>
>> ================
>> Comment at: lib/Analysis/LoopAccessAnalysis.cpp:292
>> @@ -291,2 +291,3 @@
>>        if (hasComputableBounds(SE, StridesMap, Ptr) &&
>> -          // When we run after a failing dependency check we have to
>> make sure we
>> +          // When we run after a failing dependency check we have to
>> make sure
>> +          // we
>> ----------------
>> echristo wrote:
>> > Formatting. :)
>> That was clang-format, it probably gets over 80 lines without the line
>> break. I'll rephrase the comment to save what is needed to fit in the line.
>>
>>
> Yep. Figured, it does that to me all the time, just calling it out.
>
> -eric
>
>
>> http://reviews.llvm.org/D8158
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150309/ce40830c/attachment.html>