[PATCH] D18062: Fix Load Control Dependence in MemCpy Generation

[James Y Knight via llvm-commits]

jyknight added a comment.

In http://reviews.llvm.org/D18062#381815, @hfinkel wrote:

> In http://reviews.llvm.org/D18062#381709, @niravd wrote:
>
> > Ping. Your thoughts on this Hal?
>
>
> This is quite interesting. The question is: Can we guarantee that all data dependencies are also encoded as chain dependencies? If so, then I'm happy with this -- although I'd also like some loud comments (maybe in include/llvm/CodeGen/ISDOpcodes.h), and ideally some verification code which can be enabled under expensive checks.


*NO* we can't, as is exemplified by newly reported issue in http://reviews.llvm.org/D18336 (which is broken both before and after this change). However, what we *should* guarantee is that memory dependencies are always encoded by the chain, and not indirectly through value dependencies. That was what was happening before, in the memcpy expansion, and what ought not happen.

To fix that other issue, we will need to ensure none of the stores being merged are a predecessor of any others.

> My worry here is not just about what we generate, but about what FindBetterChain might do. It would need to specifically not remove a chain dependency that was present to protect/represent a value dependency.


Well, this code being removed here did not actually try to prevent that. As it turns out, neither did anything else. (But that's another bug!)

I think the chain should not need to represent value dependencies (it doesn't now), but neither should value dependencies represent memory ordering dependencies (and this hack added because memcpy was using them that way).

> > ppc64-align-long-double.ll now may see multiple serializations of its stores

> 

> 

> What does this mean? I see you've put in CHECK-DAGs, but do you mean the order has changed or that there are now multiple stores?



http://reviews.llvm.org/D18062