Yes, I'm not arguing that there is a dependence, just that it's not a clobber dependence. The case of a load is already considered earlier in that function and with isLoad == false it returns MemDepResult::getDef(). My question is: why should a read-only call (which yields AliasAnalysis::Ref and is handled in this code fragment) be any different from e.g. a load. Isn't a read-only call effectively just a series of loads from a memory-dependence perspective? In other words, why does this code fragment return MemDepResult::getClobber() instead of MemDepResult::getDef() for a read-only call? On Sat, Jul 17, 2010 at 6:18 PM, Eugene Toder <eltoder at gmail.com> wrote:> Since isLoad == false means we're looking at a store, what this does > is making the store *p depend on the load *p. This is correct -- you > can't move store before load, otherwise load will start returning a > different value. > > Eugene > > On Fri, Jul 16, 2010 at 5:43 PM, Marc de Kruijf <dekruijf at cs.wisc.edu> > wrote: > > Hello, > > > > I'm taking a really good look at the MemoryDependenceAnalysis pass, but > I'm > > slightly confused about one little thing. I think it's a bug but I'm not > > confident enough to submit a bug report. > > > > Is there a reason why read-only calls are considered to "clobber" > pointers > > in the non-load case (which is what gets returned due to the fall-through > in > > the switch -- see below). It seems this should be returning a "def" > > instead. My thinking is the code should look like this (from line 288 > on, > > +++ marks addition): > > > > // See if this instruction (e.g. a call or vaarg) mod/ref's the > > pointer. > > switch (AA->getModRefInfo(Inst, MemPtr, MemSize)) { > > case AliasAnalysis::NoModRef: > > // If the call has no effect on the queried pointer, just ignore > it. > > continue; > > case AliasAnalysis::Mod: > > // If we're in an invariant region, we can ignore calls that ONLY > > // modify the pointer. > > if (InvariantTag) continue; > > return MemDepResult::getClobber(Inst); > > case AliasAnalysis::Ref: > > // If the call is known to never store to the pointer, and if this > is > > a > > // load query, we can safely ignore it (scan past it). > > if (isLoad) > > continue; > > +++ return MemDepResult::getDef(Inst); > > default: > > // Otherwise, there is a potential dependence. Return a clobber. > > return MemDepResult::getClobber(Inst); > > } > > > > If this seems right to you too, I've attached the patch. If this isn't > > right, can someone please explain the logic? > > > > Thanks, > > Marc > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100717/bde953fa/attachment.html>
Sorry, I misunderstood the question. The difference between a load and a read-only call is that load can be used as the value of the memory location. E.g. DeadStoreElimination pass removes a store that stores a just loaded value back into the same location. To do this it checks if the stored value is the value of load. Read-only call cannot be used like this. This being said, I don't know if this is the reason for the current logic, and DSE won't break with your change -- it checks if Def is indeed a load. A related question -- it looks like memdep returns Def for store to load dependence even when they MayAlias. Should it return Clobber for MayAlias and Def for MustAlias? With the current logic DSE has to check that SI->getPointerOperand() == DepLoad->getPointerOperand(), though this is not strictly necessary -- they can be different values but be MustAlias. Eugene On Sun, Jul 18, 2010 at 2:50 AM, Marc de Kruijf <dekruijf at cs.wisc.edu> wrote:> Yes, I'm not arguing that there is a dependence, just that it's not a > clobber dependence. The case of a load is already considered earlier in > that function and with isLoad == false it returns MemDepResult::getDef(). > My question is: why should a read-only call (which yields > AliasAnalysis::Ref and is handled in this code fragment) be any different > from e.g. a load. Isn't a read-only call effectively just a series of loads > from a memory-dependence perspective? > In other words, why does this code fragment return > MemDepResult::getClobber() instead of MemDepResult::getDef() for a read-only > call? > On Sat, Jul 17, 2010 at 6:18 PM, Eugene Toder <eltoder at gmail.com> wrote: >> >> Since isLoad == false means we're looking at a store, what this does >> is making the store *p depend on the load *p. This is correct -- you >> can't move store before load, otherwise load will start returning a >> different value. >> >> Eugene >> >> On Fri, Jul 16, 2010 at 5:43 PM, Marc de Kruijf <dekruijf at cs.wisc.edu> >> wrote: >> > Hello, >> > >> > I'm taking a really good look at the MemoryDependenceAnalysis pass, but >> > I'm >> > slightly confused about one little thing. I think it's a bug but I'm >> > not >> > confident enough to submit a bug report. >> > >> > Is there a reason why read-only calls are considered to "clobber" >> > pointers >> > in the non-load case (which is what gets returned due to the >> > fall-through in >> > the switch -- see below). It seems this should be returning a "def" >> > instead. My thinking is the code should look like this (from line 288 >> > on, >> > +++ marks addition): >> > >> > // See if this instruction (e.g. a call or vaarg) mod/ref's the >> > pointer. >> > switch (AA->getModRefInfo(Inst, MemPtr, MemSize)) { >> > case AliasAnalysis::NoModRef: >> > // If the call has no effect on the queried pointer, just ignore >> > it. >> > continue; >> > case AliasAnalysis::Mod: >> > // If we're in an invariant region, we can ignore calls that ONLY >> > // modify the pointer. >> > if (InvariantTag) continue; >> > return MemDepResult::getClobber(Inst); >> > case AliasAnalysis::Ref: >> > // If the call is known to never store to the pointer, and if >> > this is >> > a >> > // load query, we can safely ignore it (scan past it). >> > if (isLoad) >> > continue; >> > +++ return MemDepResult::getDef(Inst); >> > default: >> > // Otherwise, there is a potential dependence. Return a clobber. >> > return MemDepResult::getClobber(Inst); >> > } >> > >> > If this seems right to you too, I've attached the patch. If this isn't >> > right, can someone please explain the logic? >> > >> > Thanks, >> > Marc >> > >> > _______________________________________________ >> > LLVM Developers mailing list >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> > > >
On Sun, Jul 18, 2010 at 5:40 PM, Eugene Toder <eltoder at gmail.com> wrote:> Sorry, I misunderstood the question. > The difference between a load and a read-only call is that load can be > used as the value of the memory location. E.g. DeadStoreElimination > pass removes a store that stores a just loaded value back into the > same location. To do this it checks if the stored value is the value > of load. Read-only call cannot be used like this. > This being said, I don't know if this is the reason for the current > logic, and DSE won't break with your change -- it checks if Def is > indeed a load. >Well as a matter of consistency I think a Def is more appropriate. DSE is just one consumer of memdep analysis, and the analysis I have in mind does not use memdep in the way that DSE or GVN does.> > A related question -- it looks like memdep returns Def for store to > load dependence even when they MayAlias. Should it return Clobber for > MayAlias and Def for MustAlias? With the current logic DSE has to > check that SI->getPointerOperand() == DepLoad->getPointerOperand(), > though this is not strictly necessary -- they can be different values > but be MustAlias. >Personally, I find the Def vs. Clobber distinction rather arbitrary and not very intuitive. A may-alias store "clobbers" but a must-alias store is a "def"? I understand why there is a distinction, but the naming is poor. The comments say the naming is "unusual but pragmatic", but I have to disagree and say that the information is pragmatic only for a particular set of consumer analyses/transformations, and not across all possible consumers. As an aside, a newcomer's understanding is also compromised trying to compress everything into the two bits inside MemDepResult. I understand the desire to meet this constraint, but with three bits you could represent may/must-alias with one bit, ref/mod/invalid/nonlocal with the other two bits and it would be easier to work with.> > Eugene > > On Sun, Jul 18, 2010 at 2:50 AM, Marc de Kruijf <dekruijf at cs.wisc.edu> > wrote: > > Yes, I'm not arguing that there is a dependence, just that it's not a > > clobber dependence. The case of a load is already considered earlier in > > that function and with isLoad == false it returns MemDepResult::getDef(). > > My question is: why should a read-only call (which yields > > AliasAnalysis::Ref and is handled in this code fragment) be any different > > from e.g. a load. Isn't a read-only call effectively just a series of > loads > > from a memory-dependence perspective? > > In other words, why does this code fragment return > > MemDepResult::getClobber() instead of MemDepResult::getDef() for a > read-only > > call? > > On Sat, Jul 17, 2010 at 6:18 PM, Eugene Toder <eltoder at gmail.com> wrote: > >> > >> Since isLoad == false means we're looking at a store, what this does > >> is making the store *p depend on the load *p. This is correct -- you > >> can't move store before load, otherwise load will start returning a > >> different value. > >> > >> Eugene > >> > >> On Fri, Jul 16, 2010 at 5:43 PM, Marc de Kruijf <dekruijf at cs.wisc.edu> > >> wrote: > >> > Hello, > >> > > >> > I'm taking a really good look at the MemoryDependenceAnalysis pass, > but > >> > I'm > >> > slightly confused about one little thing. I think it's a bug but I'm > >> > not > >> > confident enough to submit a bug report. > >> > > >> > Is there a reason why read-only calls are considered to "clobber" > >> > pointers > >> > in the non-load case (which is what gets returned due to the > >> > fall-through in > >> > the switch -- see below). It seems this should be returning a "def" > >> > instead. My thinking is the code should look like this (from line 288 > >> > on, > >> > +++ marks addition): > >> > > >> > // See if this instruction (e.g. a call or vaarg) mod/ref's the > >> > pointer. > >> > switch (AA->getModRefInfo(Inst, MemPtr, MemSize)) { > >> > case AliasAnalysis::NoModRef: > >> > // If the call has no effect on the queried pointer, just > ignore > >> > it. > >> > continue; > >> > case AliasAnalysis::Mod: > >> > // If we're in an invariant region, we can ignore calls that > ONLY > >> > // modify the pointer. > >> > if (InvariantTag) continue; > >> > return MemDepResult::getClobber(Inst); > >> > case AliasAnalysis::Ref: > >> > // If the call is known to never store to the pointer, and if > >> > this is > >> > a > >> > // load query, we can safely ignore it (scan past it). > >> > if (isLoad) > >> > continue; > >> > +++ return MemDepResult::getDef(Inst); > >> > default: > >> > // Otherwise, there is a potential dependence. Return a > clobber. > >> > return MemDepResult::getClobber(Inst); > >> > } > >> > > >> > If this seems right to you too, I've attached the patch. If this > isn't > >> > right, can someone please explain the logic? > >> > > >> > Thanks, > >> > Marc > >> > > >> > _______________________________________________ > >> > LLVM Developers mailing list > >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >> > > >> > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100718/d6ba33ac/attachment.html>