[PATCH] D10749: DAGCombiner: Assume invariant load cannot alias a store

[hfinkel at anl.gov]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13821
@@ +13820,3 @@
+  // they cannot alias.
+  if (Op0->isInvariant() && Op1->writeMem())
+    return false;
----------------
arsenm wrote:
> hfinkel wrote:
> > arsenm wrote:
> > > reames wrote:
> > > > Not sure about this, but is writeMem correct here?  At least most places, we use a conservative notion for writes.  i.e. it *may* write, not it *must* write.  If we had an instruction which may write, but actually just reads in this context, the aliasing could be wrong.  
> > > > 
> > > > !mayLoad might be a possibility.
> > > The mayLoad/mayStore are properties on a MachineInstr, but these are DAG nodes which I don't think have the same concept.
> > The situation here is somewhat subtle. writeMem delegates to MachineMemOperand::isStore(), which checks whether the MOStore flag is set. What has this flag set? For the most part, just actual stores (trunc stores, atomic stores, etc.). However, this does include the MMOs on masked stores (which might or might not actually store anything), and also include all memory intrinsic nodes that might write to memory. The intrinsics include things like ISD::PREFETCH (which is tagged as writing for dependency reasons, but never writes anything). In short, care is required. But I think this does not hurt anything assuming you special-case ISD::PREFETCH here, so please do that.
> > 
> > 
> I don't think prefetch matters here. isAlias's operands are LSBaseSDNode, so this seems to only handle loads and stores. It seems ISD::PREFETCH is a MemSDNode
Ah, okay. In that case, then yes, it does not matter here. You may want to add a comment to this effect, however, just in case someone later decides to extend isAlias to handle intrinsics too. That does not seem unlikely. I'm not insisting on it.



http://reviews.llvm.org/D10749