[llvm] r242281 - [PM/AA] Fix numerous serious bugs in GlobalsModRef found by

Wed Jul 15 17:24:36 PDT 2015

Thanks!

On Wed, Jul 15, 2015 at 5:23 PM Sean Silva <chisophugis at gmail.com> wrote:

> Added in r242356.
>
> -- Sean Silva
>
> On Wed, Jul 15, 2015 at 3:49 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Wed, Jul 15, 2015 at 2:18 PM, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>>
>>> Awesome! Do you want to land that test case? If not, I will, but you may
>>> get to it sooner at this point...
>>>
>>
>> Sure, I'll do that now.
>>
>> -- Sean Silva
>>
>>
>>>
>>> On Wed, Jul 15, 2015 at 2:08 PM Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>> Thanks! I think this fixed PR23345; I didn't know about
>>>> I.mayWriteToMemory()
>>>>
>>>> -- Sean Silva
>>>>
>>>> On Wed, Jul 15, 2015 at 1:53 AM, Chandler Carruth <chandlerc at gmail.com>
>>>> wrote:
>>>>
>>>>> Author: chandlerc
>>>>> Date: Wed Jul 15 03:53:29 2015
>>>>> New Revision: 242281
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=242281&view=rev
>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D242281-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v_2U7OQd83CL4gchkkSZVU3fb0tMVRRG0j7MxIm2l1Q&s=FIjAFu91y9yQQlNOxBR3MNPtBoxckgBquFpcazgFFcc&e=>
>>>>
>>>>
>>>>> Log:
>>>>> [PM/AA] Fix *numerous* serious bugs in GlobalsModRef found by
>>>>> inspection.
>>>>>
>>>>> While we want to handle calls specially in this code because they
>>>>> should
>>>>> have been modeled by the call graph analysis that precedes it, we
>>>>> should
>>>>> *not* be re-implementing the predicates for whether an instruction
>>>>> reads
>>>>> or writes memory. Those are well defined already. Notably, at least the
>>>>> following issues seem to be clearly missed before:
>>>>> - Ordered atomic loads can "write" to memory by causing writes from
>>>>> other
>>>>>   threads to become visible. Similarly for ordered atomic stores.
>>>>> - AtomicRMW instructions quite obviously both read and write to memory.
>>>>> - AtomicCmpXchg instructions also read and write to memory.
>>>>> - Fences read and write to memory.
>>>>> - Invokes of intrinsics or memory allocation functions.
>>>>>
>>>>> I don't have any test cases, and I suspect this has never really come
>>>>> up
>>>>> in the real world. But there is no reason why it wouldn't, and it makes
>>>>> the code simpler to do this the right way.
>>>>>
>>>>> While here, I've tried to make the loops significantly simpler as well
>>>>> and added helpful comments as to what is going on.
>>>>>
>>>>> Modified:
>>>>>     llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp
>>>>>
>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp?rev=242281&r1=242280&r2=242281&view=diff
>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Analysis_IPA_GlobalsModRef.cpp-3Frev-3D242281-26r1-3D242280-26r2-3D242281-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=v_2U7OQd83CL4gchkkSZVU3fb0tMVRRG0j7MxIm2l1Q&s=Q0Y9JUhRV2iEM7NdBl52tiz7eVvdznT8n2zfuYufLF0&e=>
>>>>
>>>>
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp (original)
>>>>> +++ llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp Wed Jul 15 03:53:29
>>>>> 2015
>>>>> @@ -439,30 +439,39 @@ void GlobalsModRef::AnalyzeCallGraph(Cal
>>>>>      }
>>>>>
>>>>>      // 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)
>>>>> -        if (LoadInst *LI = dyn_cast<LoadInst>(&*II)) {
>>>>> +    for (auto *Node : SCC) {
>>>>> +      if (FunctionEffect == ModRef)
>>>>> +        break; // The mod/ref lattice saturates here.
>>>>> +      for (Instruction &I : inst_range(Node->getFunction())) {
>>>>> +        if (FunctionEffect == ModRef)
>>>>> +          break; // The mod/ref lattice saturates here.
>>>>> +
>>>>> +        // We handle calls specially because the graph-relevant
>>>>> aspects are
>>>>> +        // handled above.
>>>>> +        if (auto CS = CallSite(&I)) {
>>>>> +          if (isAllocationFn(&I, TLI) || isFreeCall(&I, TLI)) {
>>>>> +            // FIXME: It is completely unclear why this is necessary
>>>>> and not
>>>>> +            // handled by the above graph code.
>>>>> +            FunctionEffect |= ModRef;
>>>>> +          } else if (Function *Callee = CS.getCalledFunction()) {
>>>>> +            // The callgraph doesn't include intrinsic calls.
>>>>> +            if (Callee->isIntrinsic()) {
>>>>> +              ModRefBehavior Behaviour =
>>>>> +                  AliasAnalysis::getModRefBehavior(Callee);
>>>>> +              FunctionEffect |= (Behaviour & ModRef);
>>>>> +            }
>>>>> +          }
>>>>> +          continue;
>>>>> +        }
>>>>> +
>>>>> +        // All non-call instructions we use the primary predicates
>>>>> for whether
>>>>> +        // thay read or write memory.
>>>>> +        if (I.mayReadFromMemory())
>>>>>            FunctionEffect |= Ref;
>>>>> -          if (LI->isVolatile())
>>>>> -            // Volatile loads may have side-effects, so mark them as
>>>>> writing
>>>>> -            // memory (for example, a flag inside the processor).
>>>>> -            FunctionEffect |= Mod;
>>>>> -        } else if (StoreInst *SI = dyn_cast<StoreInst>(&*II)) {
>>>>> +        if (I.mayWriteToMemory())
>>>>>            FunctionEffect |= Mod;
>>>>> -          if (SI->isVolatile())
>>>>> -            // Treat volatile stores as reading memory somewhere.
>>>>> -            FunctionEffect |= Ref;
>>>>> -        } else if (isAllocationFn(&*II, TLI) || isFreeCall(&*II,
>>>>> TLI)) {
>>>>> -          FunctionEffect |= ModRef;
>>>>> -        } else if (IntrinsicInst *Intrinsic =
>>>>> dyn_cast<IntrinsicInst>(&*II)) {
>>>>> -          // The callgraph doesn't include intrinsic calls.
>>>>> -          Function *Callee = Intrinsic->getCalledFunction();
>>>>> -          ModRefBehavior Behaviour =
>>>>> AliasAnalysis::getModRefBehavior(Callee);
>>>>> -          FunctionEffect |= (Behaviour & ModRef);
>>>>> -        }
>>>>> +      }
>>>>> +    }
>>>>>
>>>>>      if ((FunctionEffect & Mod) == 0)
>>>>>        ++NumReadMemFunctions;
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150716/a9e826fe/attachment.html>

[llvm] r242281 - [PM/AA] Fix *numerous* serious bugs in GlobalsModRef found by

[llvm] r242281 - [PM/AA] Fix numerous serious bugs in GlobalsModRef found by