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

[Daniel Berlin]

Okay, i can get this to fail (the getmodrefinfo for cmpxchgb should be
checking whether Loc.Ptr is null before calling alias0


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

> It was on mac. I've attached the exact diff on top of my previous patch.
>
> On Mon, Apr 27, 2015 at 3:14 PM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>> I cannot get it to crash on the testcase you have provided with the
>> change I suggested.
>>
>> What platform did you try this on?
>>
>>
>> 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/66143340/attachment.html>