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

[Daniel Berlin]

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.

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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150427/bee50d2c/attachment.html>