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

[Hans Wennborg]

Do you think this fix and test case should be merged to the 3.7 branch?

On Wed, Jul 15, 2015 at 5:24 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> 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
>>>>>>
>>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>