[PATCH] [globalsmodref-aa] Atomics *DO* touch memory...

[Hal Finkel]

----- Original Message -----
> From: "Daniel Berlin" <dberlin at dberlin.org>
> To: "Sean Silva" <chisophugis at gmail.com>, "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Nick Lewycky" <nlewycky at google.com>, "Philip Reames" <listmail at philipreames.com>, llvm-commits at cs.uiuc.edu
> Sent: Monday, April 27, 2015 3:34:54 PM
> Subject: Re: [PATCH] [globalsmodref-aa] Atomics *DO* touch memory...
> 
> 
> Refactoring should be easy.
> 
> The whole set of blocks can be replaced with:
> 
> 
> // Scan the function bodies for explicit loads or stores.
> for (unsigned i = 0, e = SCC.size(); i != e && FunctionEffect !=
> ModRef;++i)
> for (inst_iterator II = inst_begin(SCC[i]->getFunction()),
> E = inst_end(SCC[i]->getFunction());
> II != E && FunctionEffect != ModRef; ++II)
> {
> FunctionEffect |= getModRefInfo(&*II);
> }
> 
> 
> 
> This should fix your bug too (your code misses FenceInst, VAArgInst,
> and a few others) ;)
> 
> 
> The only one i'm not 100% positive this covers is isAllocationFn and
> isFreeFn, but we should get the right answer here anyway if i'm
> following call chains right.

My thought too, which is why I did not understand why there were being handled separately.

 -Hal

> 
> 
> On Mon, Apr 27, 2015 at 1:16 PM Sean Silva < chisophugis at gmail.com >
> wrote:
> 
> 
> 
> 
> 
> On Sat, Apr 25, 2015 at 2:21 AM, Hal Finkel < hfinkel at anl.gov >
> wrote:
> 
> 
> 
> 
> ----- Original Message -----
> > From: "Sean Silva" < chisophugis at gmail.com >
> > To: llvm-commits at cs.uiuc.edu
> > Cc: "Hal Finkel" < hfinkel at anl.gov >, "Nick Lewycky" <
> > nlewycky at google.com >, "Philip Reames" < listmail at philipreames.com
> > >
> > Sent: Friday, April 24, 2015 10:56:58 PM
> > Subject: [PATCH] [globalsmodref-aa] Atomics *DO* touch memory...
> > 
> > 
> > GlobalsModRef was giving a spurious alias analysis result because
> > it
> > failed to consider that atomics access memory. The attached patch
> > fixes that (PR23345).
> > 
> > 
> > I reduced this from a "compare and swap" loop that looked funny in
> > the machine code: the "compare and swap" had in fact been hoisted
> > out of the loop. I only ran into this when I tried `-flto -O2
> > -fno-inline`.
> > 
> > 
> > The structure of the test is pretty closely copied from one of the
> > others in the directory. Any better ideas for testing this (or ways
> > to make the testing more thorough) would be appreciated.
> > 
> > 
> > If somebody can doublecheck that these are the only missing cases,
> > that would be appreciated. This loop been touched since 2012 and
> > I'm
> > relatively clueless about AA. Hopefully this patch also suggests a
> > couple other places to audit for similar bugs to those with a
> > better
> > "big picture" of the IR analysis and transforms than myself.
> 
> Hi Sean,
> 
> The list is also missing VAArgInst, although maybe that's on purpose
> (although, in that case, a comment would be nice).
> 
> While we're looking at this, other things I don't understand about
> this loop are 1) Why is is special casing alloc/free functions and
> 2) why is it treating all volatile accesses as read/write?
> 
> 
> 
> 
> 
> 
> I know at least for volatile accesses it is because it might be a
> hardware register that has some arbitrary effect on other memory. A
> simple example is ARM's "bit-banding".
> 
> 
> 
> 
> 
> 
> 
> 
> It might be nice to refactor this code to call
> ModRefResult getModRefInfo(const Instruction *I)
> from AliasAnalysis. That way we could reduce the number of places
> that replicate this kind of logic.
> 
> 
> 
> 
> 
> 
> 
> 
> Hal, others: I'm really not an expert in AA or even really have that
> much experience at the IR level. Would one of you be willing to
> comandeer this patch from me? I'm afraid that I'm mostly going to be
> slowing things down here.
> 
> 
> 
> 
> 
> -- Sean Silva
> 
> 
> 
> 
> 
> 
> -Hal
> 
> > 
> > 
> > -- Sean Silva
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory