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

[Sean Silva]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150715/21edf2e4/attachment.html>