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

Fri Jul 17 13:20:06 PDT 2015

LGTM to merge.

On Fri, Jul 17, 2015 at 1:02 PM Hans Wennborg <hans at chromium.org> wrote:

> 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
> >
> _______________________________________________
> 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/20150717/1f0f06f7/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