[PATCH] D18546: Prevent X86IselLowering from merging volatile loads

[James Y Knight via llvm-commits]

jyknight added a comment.

> And here's the resulting codegen after applying http://reviews.llvm.org/D18062 and http://reviews.llvm.org/D14834:

> 

>   vmovss	8(%rdi), %xmm0       <--- this is the load volatile float specified by the IR

>   vmovups	8(%rdi), %xmm0       <--- this is a 16-byte merged load that includes the volatile region

>    

> 

> We still have the volatile access specified by the IR. I'm guessing this is wrong though because we've accessed the volatile region a 2nd time. The LangRef seems vague to me about this:

>  http://llvm.org/docs/LangRef.html#volatile-memory-accesses


I think that code is wrong because the *value* is coming from the second load, not the first. The code says to do a 4-byte volatile read of the pointer, and so it should do exactly that, and use that value, not something else. The 4-byte load is only being preserved through the chain.

Of course, at least in some circumstances, it's okay to widen a normal load to cover memory that wasn't explicitly loaded (e.g. with undef in the middle of the vector). And obviously, if it was okay to widen a normal load *without* the volatile load being present, adding a volatile load in the middle wouldn't make that transform suddenly invalid. So, with a slightly different test case (e.g. if the second pointer was volatile), you could potentially validly generate a merged 16-byte load that overlapped the volatile region, a separate 4-byte volatile load, and then combine those to the result vector. But even so

> Assuming the extra load of the volatile region is wrong, I would prefer that the check for volatility goes in isConsecutiveLoad(). It could be triggered by an optional bool param. If we only check volatility in EltsFromConsecutiveLoads(), I'm scared that other targets may have already mutated that code and wouldn't get the fix.


That makes sense to me. The only other caller in-tree of this function already checks isVolatile, anyways.


http://reviews.llvm.org/D18546