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

Mon Apr 27 15:36:00 PDT 2015

Here's a fixed patch.

I don't have time to write enough testcases for it right now, but it fixes
the testcases you provided :)

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

> 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/35dfa7ea/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedglobalsmodref.patch
Type: application/octet-stream
Size: 2447 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150427/35dfa7ea/attachment.obj>