[PATCH] D8688: Update MergedLoadStoreMotion to use MemorySSA

[Geoff Berry via llvm-commits]

On 7/5/2016 8:21 PM, Daniel Berlin wrote:
>
>
> On Wed, Jul 6, 2016 at 1:48 AM, Geoff Berry <gberry at codeaurora.org 
> <mailto:gberry at codeaurora.org>> wrote:
>
>     gberry added a comment.
>
>     Sorry if this isn't the right place, but I have a general question
>     about what it means to preserve MemorySSA, specifically regarding
>     the defining accesses of MemoryUse nodes.  Is the idea here that
>     we make a best effort to keep the MemoryUse defining access links
>     optimized (i.e. never pointing to a no-alias def)?
>
>
> Yes. You should fix anything obviously affected/invalidated (this is a 
> step past what memdep does, which is mostly nothing). But you should 
> not have to redo the links themselves except those related to the 
> load/store you touched.
>
> The *cache* (if any), needs more invalidation, however.
> So if you destroy a load, that means you pretty much have to do nothing.
> If you destroy a store, you either need fine-grained tracking, or 
> destroy the entire cache.

Unless I'm missing something, this doesn't jibe with the caching walker 
changes under review (http://reviews.llvm.org/D21777).  If the walker 
doesn't cache MemoryUse clobbering accesses (assuming that a MemoryUse's 
defining access is always the "best" clobber), then invalidating the 
cache when e.g. destroying a store isn't good enough since the cache is 
never consulted when querying about a MemoryUse's clobber.

>
>     Because of the limits of basic-aa, it isn't possible to guarantee
>     this property after any code transformation, even in the limited
>     case here, since the alias results for completely unrelated
>     load/stores may have been affected, right?
>
> For the moment, yes, but this is 100% brokenness in basicaa that 
> should be fixed, and will eventually be fixed.
>
> There is simply never a good reason to have these limits in basicaa, 
> they are symptomatic of basicaa having random stateless backwards 
> walkers for properties that can be computed forwards and cached.
> Everything that has such random backwards-walking limits should be 
> moved out into a caching pass of some sort that is appropriately 
> invalidated or recomputed.
> Lest one worry, they pretty much never need to be recomputed, or at 
> most, once (after serious loop optimizations).  Nothing changes their 
> values because it would change the semantics of the program. Most of 
> the time they simply reuse computations but still compute the same end 
> result :P
>
> In any case, your compiler's alias analysis results should not change 
> at a basic level simply because you moved from 99 lines of code to 100 
> lines (and llvm's is the only one i know which does :( ).
>
> BasicAA is also being used to do *way* too much.   This is a symptom 
> of never having any real pointer analysis at all, and so people use 
> basicaa to try to figure out pointer analysis problems.
>
> All of this is a  borked design at this point.  BasicAA started off 
> with good intentions, but it needs a lot of love at this point :)
>
>
>     http://reviews.llvm.org/D8688
>
>
>
>

-- 
Geoff Berry
Employee of Qualcomm Innovation Center, Inc.
  Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160706/46fe0a36/attachment.html>