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

Sean Silva chisophugis at gmail.com
Mon Apr 27 15:17:58 PDT 2015


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/d33cde59/attachment.html>
-------------- next part --------------
diff --git a/lib/Analysis/IPA/GlobalsModRef.cpp b/lib/Analysis/IPA/GlobalsModRef.cpp
index c4660fa..e550ecf 100644
--- a/lib/Analysis/IPA/GlobalsModRef.cpp
+++ b/lib/Analysis/IPA/GlobalsModRef.cpp
@@ -439,7 +439,10 @@ void GlobalsModRef::AnalyzeCallGraph(CallGraph &CG, Module &M) {
     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)
+           II != E && FunctionEffect != ModRef; ++II) {
+        FunctionEffect |= AliasAnalysis::getModRefInfo(&*II);
+      }
+#if 0
         if (LoadInst *LI = dyn_cast<LoadInst>(&*II)) {
           FunctionEffect |= Ref;
           if (LI->isVolatile())
@@ -461,6 +464,7 @@ void GlobalsModRef::AnalyzeCallGraph(CallGraph &CG, Module &M) {
         } else if (isa<AtomicCmpXchgInst>(&*II) || isa<AtomicRMWInst>(&*II)) {
           FunctionEffect |= ModRef;
         }
+#endif
 
     if ((FunctionEffect & Mod) == 0)
       ++NumReadMemFunctions;


More information about the llvm-commits mailing list