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

[Daniel Berlin]

I can try, but i am not going to have a ton of time this week.


On Mon, Apr 27, 2015 at 2:56 PM Sean Silva <chisophugis at gmail.com> wrote:

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