[PATCH] D31726: AliasAnalysis: Be less conservative about volatile than atomic.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 4 inline comments as done.
dberlin added inline comments.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:336
+  // Be conservative in the face of atomic.
+  if (isStrongerThan(L->getOrdering(), AtomicOrdering::NotAtomic))
     return MRI_ModRef;
----------------
sanjoy wrote:
> dberlin wrote:
> > dberlin wrote:
> > > sanjoy wrote:
> > > > Why not `isStrongerThan(L->getOrdering(), AtomicOrdering::Unordered)`? (same below)
> > > > 
> > > > OTOH I'm surprised that this did not break any tests.
> > > Honestly:
> > > Because i don't know enough to know if that's okay ;)
> > > If it is, happy to do it.
> > > 
> > > It breaks 1 optimizations test that i'm looking at:
> > > 
> > > memcpy/atomic.ll optimizes *less* now.
> > > Which makes no sense at all.  I suspect it was relying on weird aliasing before. I'm debugging.
> > > 
> > > Otherwise, it does break some memoryssa tests, which i'm updating for the correct answers.
> > > (i'm also still awaiting 3 stage results on linux)
> > > 
> > Actually the memcpyopt is exactly the issue you point out, so fixing as part of test updates.
> I think it is, but (more importantly) wasn't that the behavior before?
> 
> Previously if `L` was non-volatile AND (NotAtomic or Unordered) then we'd go through the normal alias analysis codepath.
> 
> Now you want to remove the "non-volatile AND" bit.  This would mean we'd want to go through the normal path if the load was NotAtomic or Unordered.
Yes, you are correct., fixed.



https://reviews.llvm.org/D31726