[PATCH] D23214: [MDA] Treat invariant.start as non-dependence

[Sanjoy Das via llvm-commits]

Hi Daniel,

Daniel Berlin wrote:
 > Yes, i'm saying the  mechanism currently used to keep them from moving
 > is to say they read/write memory.
 >
 >     That
 >     does not seem safe -- how does LLVM know that they won't segfault?
 >
 >
 >
 > I'm not sure i understand.

I meant given a call that is readonly, LLVM cannot generally hoist it
out of control flow unless it has special knowledge that the call does
not read invalid memory.  Otherwise it will introduce undefined
behavior:

   if (always_false)
     readonly_call_that_derefs_null()

==>

   readonly_call_that_derefs_null()
   if (always_false)
     ...

 > These are invariant start and end markers, they generate no real code.
 > If you move them, the problem is that the definition of invariant.start
 > says:
 > "This intrinsic indicates that until an|llvm.invariant.end|that uses the
 > return value, the referenced memory location is constant and unchanging.
 > "
 >
 > So if you hoist/sink start or end , you change the bounds of where the
 > memory is unchanging.

Yes. I can see why cannot move _stores_ across them, since


   *ptr = 20
   invariant_start(ptr)

==>

   invariant_start(ptr)
   *ptr = 20


is bad -- we've moved a store to a region in which ptr needs to be
unchanging.


With the memdep changes we will allow reordering loads across the
invarinat_start call:

   int val = *ptr;
   invariant_start(ptr)

<==>

   int val = *ptr;
   invariant_start(ptr)

but that seems fine to me; that is I could not find a reason why
reordering loads across invariant_start will be a problem.  But I
don't have a deep reason why reordering loads is correct, so I won't
be surprised if there are cases that reordering is a problem.

 > You could fix this, of course, by having the invariant.start and
 > invariant.end calls return pointers, and all things that are invariant
 > use those pointers (or pointers derived from those pointers).  IE
 > express the dependence in SSA directly.

I remember a thread by Andy Trick a while back which intended to add
this to LLVM.  But that didn't result in concrete patches.

 > Then you can move the start/end calls wherever they are valid (IE
 > wherever they are dominated by their operands) and it will not change
 > the semantic effect.
 >
 > But that's not the current design.
 >
 >
 >     >  We last tried this by saying they did not touch memory (which is
 >     >  correct), and peter had to revert the set of changes that gave
 >     them "The
 >     >  correct answer" because it broke on real code.
 >
 >     Do you happen to remember any relevant revision numbers / zilla bugs
 >     for this?
 >
 > The revert was: "[llvm] r270823 - MemorySSA: Revert r269678 and r268068;
 > replace with special casing in MemorySSA.
 > "
 >
 > This is when we tried this with assume

Oh, I remember that one.  But IIUC that was not a problem with
speculation but with DCE'ing away assumes.  ReadNone can't be
speculated in LLVM either since they can contain a divide-by-zero.

I don't think that the same problem will occur here since unlike the
assume patch, we're not telling LLVM that calls to invariant_start are
readonly, but that they don't cause a dependency on any specific load
instruction in the LLVM IR.  This is sort of like saying that the
invariant_start call does write to memory, but just not to memory that
can be observed via LLVM IR.

However, I think the right place for this is in Alias Analysis, where
we handle assumes and guards.

-- Sanjoy

 > Look for emails from pcc around the time you see "[PATCH] D20653: LICM:
 > Do not sink or hoist assume intrinsic calls."
 >
 > This was a set of followups from peter to try to starting fixing places
 > that tried to hoist them anyway, and it turns out ... it's not a pretty
 > path.
 > ;)
 >
 >     -- Sanjoy
 >
 >