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

[Mehdi Amini]

> 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 <mailto: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. 
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.

 — 
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 <http://reviews.llvm.org/D8158>
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150309/7bc43065/attachment.html>