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

Tue Apr 28 12:23:28 PDT 2015

Note: I fixed all the cases where getModRefInfo was crashing on
instructions, added unit tests, and commited those fixes.

This means the change of just using getModRefInfo should just work now
(I tested it does), and all you should have to do is write tests.

On Mon, Apr 27, 2015 at 3:36 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> 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
>>>
>>>
>