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

[Hans Wennborg]

Merged in r242570.

Cheers,
Hans

On Fri, Jul 17, 2015 at 1:20 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> 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