[PATCH] D16300: Enable LoopLoadElimination by default

[Adam Nemet via llvm-commits]

> On Jan 27, 2016, at 4:58 PM, Philip Reames <listmail at philipreames.com> wrote:
> 
> 
> 
> On 01/22/2016 12:16 PM, Adam Nemet wrote:
>> anemet added a comment.
>> 
>> It seems to me that partial redundancy means slightly different things between GVN Load-PRE and LLE.
> I think that terminology wise, we actually mean the same thing by "partially redundant".  We mean there's one or more paths where it wasn't previously available where we need to make it available to remove the load candidate.  The difference is what subset of possible partially redundant cases each one handles.
> 
> (The preceding is probably a nit, just getting terminology clear.)

Yeah, you’re right.  I was overcomplicating by trying to make a distinction.  I was probably thinking of this too much in terms of recurrences.  As you say the better distinction is whether we insert loads on a single path from outside the loop or on other paths as well within the loop.

>> 
>> In LLE, a load is partially redundant, if the forwarding store does not dominate all the loop latches.  To make it fully redundant we would have to add loads on the unavailable paths.
>> 
>> I think that in GVN, the loopy case is always considered a partial redundancy case because you'd have to insert a load in the preheader.  Let me know if I am getting this wrong.
> This is likely to be true.
>> 
>> So it seems to me that the LLE partial redundancy case is equivalent to the case in GVN Load-PRE when we need *more than one* load inserted.
> This seems reasonable, but note that we currently only insert a single load when doing Load-PRE in GVN.  This is a heuristic/profitability decision, not a legality one.

Ah, thanks for the info.  So I guess then the only difference is the lack of load->load forwarding in LLE.

Also Philip, are you satisfied with my response to your question earlier?  Specifically that we don’t want to version when optimizing for size.

Thanks for the feedback,
Adam

>> 
>> Does this make sense?
>> 
>> 
>> http://reviews.llvm.org/D16300
>> 
>> 
>> 
>