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

[Sean Silva]

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

>
>
> On Mon, Apr 27, 2015 at 1:58 PM Sean Silva <chisophugis at gmail.com> wrote:
>
>> 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.
>>
>
> If people add instructions, we always have this problem.
>
> I would actually start by adding the tests it was performing before to
> unittests/Analysis/AliasAnalysisTest (which now exists)  to make sure
> getModRefInfo returns the results it was expecting.
>
> That will test this path make sure the code path is still getting the same
> answers.
>
> Then I would add whatever tests for globalsmodref to make sure that is
> getting turned into right mod/ref results for your volatile cases.
>
>
It sounds like you have a pretty clear idea of the path forward, would you
mind comandeering the patch from me? While I'd love to dig into this as a
learning experience for this part of the codebase, right now I'm currently
neck-deep in running some performance tests.

FWIW I tried your suggested modification and it is dying with:

* thread #1: tid = 0xaf3005, 0x0000000100874297
opt`llvm::GetUnderlyingObject(llvm::Value*, llvm::DataLayout const&,
unsigned int) [inlined] llvm::Value::getType(this=0x0000000000000000) const
at Value.h:207, queue = 'com.apple.main-thread', stop reason =
EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x0000000100874297
opt`llvm::GetUnderlyingObject(llvm::Value*, llvm::DataLayout const&,
unsigned int) [inlined] llvm::Value::getType(this=0x0000000000000000) const
at Value.h:207
    frame #1: 0x0000000100874297
opt`llvm::GetUnderlyingObject(V=0x0000000000000000, DL=0x0000000102414398,
MaxLookup=6) + 23 at ValueTracking.cpp:2769
    frame #2: 0x0000000100c1ced5 opt`(anonymous
namespace)::GlobalsModRef::alias(llvm::AliasAnalysis::Location const&,
llvm::AliasAnalysis::Location const&) [inlined]
llvm::GetUnderlyingObject(MaxLookup=6) + 5 at ValueTracking.h:177
    frame #3: 0x0000000100c1ced0 opt`(anonymous
namespace)::GlobalsModRef::alias(this=0x0000000102416520,
LocA=0x00007fff5fbfefd8, LocB=0x00007fff5fbff1c0) + 64 at
GlobalsModRef.cpp:492
    frame #4: 0x0000000100724889
opt`llvm::AliasAnalysis::getModRefInfo(this=0x0000000102416540,
CX=<unavailable>, Loc=0x00007fff5fbff1c0) + 169 at AliasAnalysis.cpp:403
    frame #5: 0x0000000100c1cade opt`(anonymous
namespace)::GlobalsModRef::runOnModule(llvm::Module&) [inlined]
llvm::AliasAnalysis::getModRefInfo(this=0x0000000102416540,
I=<unavailable>) + 5118 at AliasAnalysis.h:383
    frame #6: 0x0000000100c1ca5b opt`(anonymous
namespace)::GlobalsModRef::runOnModule(llvm::Module&) + 40 at
GlobalsModRef.cpp:443
    frame #7: 0x0000000100c1ca33 opt`(anonymous
namespace)::GlobalsModRef::runOnModule(this=0x0000000102416520,
M=<unavailable>) + 4947 at GlobalsModRef.cpp:105
    frame #8: 0x0000000100bbc3f1
opt`llvm::legacy::PassManagerImpl::run(llvm::Module&) + 111 at
LegacyPassManager.cpp:1616
    frame #9: 0x0000000100bbc382
opt`llvm::legacy::PassManagerImpl::run(this=0x0000000102413ed0,
M=0x00000001024142b0) + 738 at LegacyPassManager.cpp:1723
    frame #10: 0x000000010000ff7c opt`main(argc=<unavailable>,
argv=0x00007fff5fbff830) + 7676 at opt.cpp:611
    frame #11: 0x00007fff95dcc5c9 libdyld.dylib`start + 1

on the test case in my original patch.

-- Sean Silva



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