[PATCH] D28247: [DAG] Check for preexisting store when emiting stack convert

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Is it actually okay to introduce a read from a memory location that didn't have one in the source? This might do the wrong thing if e.g. if you have two threads non-atomically writing to a global, but never reading it. I'd suspect it's not okay, because it introduces a data race, but I'm not certain.

Going back to the case this is supposed to be improving, from ppc64-align-long-double.ll in the https://reviews.llvm.org/D14834 patch. In that case, there *was* a load, but it got eliminated as unnecessary, by forwarding the value through from the store, instead. So, by the time we realize that the bitcast needs to be lowered to store then load, that information is gone.

We could change this patch to only allow introducing new loads from a spill slot -- that might handle the particular thing noticed in the above test, and not have issues with potential for incorrectness. But that seems a somewhat small special case so I'm not sure it's actually worth it. I mean -- the calling lowering of that function is already bad, with seemingly-spurious storing of registers to memory in lowering the struct* byval argument, anyways.

I'm inclined to suggest just reverting this and leaving that test with its FIXME...


https://reviews.llvm.org/D28247