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

[Sean Silva]

On Mon, Apr 27, 2015 at 1:34 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

> 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.
>


Any idea about the best way to test this? We could extend the testing in
the original patch to cover other instructions, but I feel like having the
list of instructions in the test is as fragile as open-coding the list of
instructions in the code.

-- Sean Silva


>
> 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/d360efe8/attachment.html>