[llvm-commits] [PATCH] Use GetUnderlyingObjects in misched

Sergei Larin slarin at codeaurora.org
Mon Dec 3 08:45:54 PST 2012 

It was not much - I did not keep the exact numbers, but it was somewhere under 1-2% for a pathological case.
I think the patch itself is very important and I want to see it in the tree asap... even if it lengthens the analysis time a bit.
Thanks.

Sergei

---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Friday, November 30, 2012 10:31 PM
> To: Evan Cheng
> Cc: Sergei Larin; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in misched
> 
> ----- Original Message -----
> > From: "Evan Cheng" <evan.cheng at apple.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: "Sergei Larin" <slarin at codeaurora.org>, llvm-commits at cs.uiuc.edu
> > Sent: Sunday, November 25, 2012 1:50:31 PM
> > Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> > misched
> >
> > I'm also concerned about compile time impact. Do you have some
> > numbers?
> 
> Not anything comprehensive, but I'll run some tests. We certainly may
> want to limit the recursion depth on targets that don't see any
> significant benefit.
> 
> Sergei, can you quantify your observed compile-time effect?
> 
> Thanks again,
> Hal
> 
> >
> > Thanks,
> >
> > Evan
> >
> > On Nov 20, 2012, at 11:04 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> >
> > > ----- Original Message -----
> > >> From: "Sergei Larin" <slarin at codeaurora.org>
> > >> To: "Hal Finkel" <hfinkel at anl.gov>
> > >> Cc: llvm-commits at cs.uiuc.edu, "Andrew Trick" <atrick at apple.com>
> > >> Sent: Tuesday, November 20, 2012 12:25:35 PM
> > >> Subject: RE: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> > >> misched
> > >>
> > >> Hal,
> > >>
> > >>  I tried it with Hexagon and it seems to work fine. The only
> > >>  (minor)
> > >>  concern, it seems to affect my compile time, although very
> > >> slightly.
> > >> Thank you for allowing me time for review.
> > >
> > > Not a problem, thanks for reviewing!
> > >
> > > -Hal
> > >
> > >>
> > >> Sergei
> > >>
> > >> ---
> > >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > >> hosted by The Linux Foundation
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: Hal Finkel [mailto:hfinkel at anl.gov]
> > >>> Sent: Monday, November 19, 2012 2:29 PM
> > >>> To: Sergei Larin
> > >>> Cc: llvm-commits at cs.uiuc.edu; Andrew Trick
> > >>> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> > >>> misched
> > >>>
> > >>> ----- Original Message -----
> > >>>> From: "Sergei Larin" <slarin at codeaurora.org>
> > >>>> To: "Hal Finkel" <hfinkel at anl.gov>, "Andrew Trick"
> > >>>> <atrick at apple.com>
> > >>>> Cc: llvm-commits at cs.uiuc.edu
> > >>>> Sent: Monday, November 19, 2012 2:26:01 PM
> > >>>> Subject: RE: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> > >>>> misched
> > >>>>
> > >>>> Hal,
> > >>>>
> > >>>>  Please allow me some time to take a closer look at this. (A day
> > >>>> probably).
> > >>>> Thanks.
> > >>>
> > >>> Great, thanks!
> > >>>
> > >>> -Hal
> > >>>
> > >>>>
> > >>>> Sergei
> > >>>>
> > >>>> ---
> > >>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora
> > >>>> Forum, hosted by The Linux Foundation
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > >>>>> bounces at cs.uiuc.edu] On Behalf Of Hal Finkel
> > >>>>> Sent: Sunday, November 18, 2012 11:30 PM
> > >>>>> To: Andrew Trick
> > >>>>> Cc: llvm-commits at cs.uiuc.edu
> > >>>>> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> > >>>>> misched
> > >>>>>
> > >>>>> ----- Original Message -----
> > >>>>>> From: "Andrew Trick" <atrick at apple.com>
> > >>>>>> To: "Hal Finkel" <hfinkel at anl.gov>
> > >>>>>> Cc: llvm-commits at cs.uiuc.edu
> > >>>>>> Sent: Sunday, November 18, 2012 3:08:11 AM
> > >>>>>> Subject: Re: [PATCH] Use GetUnderlyingObjects in misched
> > >>>>>>
> > >>>>>>
> > >>>>>> On Nov 17, 2012, at 1:59 AM, Hal Finkel <hfinkel at anl.gov>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> Andy, et al.,
> > >>>>>>>
> > >>>>>>> misched currently uses GetUnderlyingObject in order to break
> > >>>>>>> false load/store dependencies, and the -enable-aa-sched-mi
> > >>>>>>> feature similarly relies on GetUnderlyingObject in order to
> > >>>>>>> ensure it is safe to use the aliasing analysis.
> > >>>>>>> Unfortunately,
> > >>>>>>> GetUnderlyingObject does not recurse through phi nodes, and
> so
> > >>>>>>> (especially due to LSR) all of these mechanisms fail for
> > >>>>>>> induction-variable-dependent loads and stores inside loops.
> > >>>>>>>
> > >>>>>>> The attached patch replaces uses of GetUnderlyingObject with
> > >>>>>>> GetUnderlyingObjects (which will recurse through phi and
> > >>>>>>> select
> > >>>>>>> instructions) in misched. On in-order cores with a long
> > >>>>>>> pipline depth (such as the PPC A2), this is extremely
> > >>>>>>> important (the loop
> > >>>>> in
> > >>>>>>> the attached test case, for example, shows a ~100% speedup).
> > >>>>>>> Please review.
> > >>>>>>
> > >>>>>> Thanks Hal. This does look like it fixes a serious handicap.
> > >>>>>> A
> > >>> few
> > >>>>>> comments...
> > >>>>>>
> > >>>>>> +static void getUnderlyingObjects(const Value *V,
> > >>>>>> +                                 SmallVectorImpl<Value *>
> > >>>>>> &Objects)
> > >>>>>> {
> > >>>>>> +  SmallVector<const Value *, 4> Working(1, V);
> > >>>>>>
> > >>>>>> You also need a visited set (SmallPtrSet) for two reasons: to
> > >>>>>> terminate phi-ptrint-intptr-phi cycles, and to avoid
> > >>>>>> exponential path exploration in long if-then-else chains.
> > >>>>>
> > >>>>> Good point, thanks!
> > >>>>>
> > >>>>>>
> > >>>>>> +    GetUnderlyingObjects(const_cast<Value *>(V), Objs);
> > >>>>>> +
> > >>>>>> +    for (SmallVector<Value *, 4>::iterator I = Objs.begin(),
> > >>>>>> IE
> > >>>>>> =
> > >>>>>> Objs.end();
> > >>>>>> +         I != IE; ++I) {
> > >>>>>> +      V = *I;
> > >>>>>>
> > >>>>>> You can check if V is visited here before doing anything else.
> > >>>>>
> > >>>>> The result of getUnderlyingObjectFromInt should be checked
> > >>>>> (maybe that's what you meant).
> > >>>>>
> > >>>>>>
> > >>>>>> +      bool Found = false;
> > >>>>>> +      for (SmallVectorImpl<Value *>::iterator J =
> > >>>>>> Objects.begin(),
> > >>>>>> +           JE = Objects.end(); J != JE; ++J)
> > >>>>>> +        if (V == *J) {
> > >>>>>> +          Found = true;
> > >>>>>> +          break;
> > >>>>>> +        }
> > >>>>>> +
> > >>>>>> +      if (!Found)
> > >>>>>>
> > >>>>>> Since only visited values are pushed, you should not need this
> > >>>>>> loop.
> > >>>>>
> > >>>>> Collecting the underlying objects from different IntToPtr
> > >>>>> instructions could still yield duplicate underlying objects. On
> > >>>>> the other hand, is it important to insure that the values in
> the
> > >>>>> array are unique?
> > >>>>> (does
> > >>>>> the scheduler care about the insertion of duplicate edges?)
> > >>>>>
> > >>>>>>
> > >>>>>>     } else if (MI->mayStore()) {
> > >>>>>> -      bool MayAlias = true;
> > >>>>>> -      if (const Value *V = getUnderlyingObjectForInstr(MI,
> > >>>>>> MFI,
> > >>>>>> MayAlias)) {
> > >>>>>> +      SmallVector<std::pair<const Value *, bool>, 4> Objs;
> > >>>>>> +      getUnderlyingObjectsForInstr(MI, MFI, Objs);
> > >>>>>> +
> > >>>>>> +      if (Objs.empty()) {
> > >>>>>> +        // Treat all other stores conservatively.
> > >>>>>> +        goto new_alias_chain;
> > >>>>>> +      }
> > >>>>>> +
> > >>>>>> +      for (SmallVector<std::pair<const Value *, bool>,
> > >>>>>> 4>::iterator
> > >>>>>> +           K = Objs.begin(), KE = Objs.end(); K != KE; ++K)
> > >>>>>> {
> > >>>>>>
> > >>>>>> For stores, we only need to add edges to pending loads, the
> > >>>>>> AliasChain, and the BarrierChain once, not for every
> > >>>>>> identifiable object.
> > >>>>>
> > >>>>> Makes sense.
> > >>>>>
> > >>>>> I've attached an updated version.
> > >>>>>
> > >>>>> Thanks again,
> > >>>>> Hal
> > >>>>>
> > >>>>>> It looks like you do the right thing for loads already.
> > >>>>>>
> > >>>>>> -Andy
> > >>>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Hal Finkel
> > >>>>> Postdoctoral Appointee
> > >>>>> Leadership Computing Facility
> > >>>>> Argonne National Laboratory
> > >>>>
> > >>>>
> > >>>
> > >>> --
> > >>> Hal Finkel
> > >>> Postdoctoral Appointee
> > >>> Leadership Computing Facility
> > >>> Argonne National Laboratory
> > >>
> > >>
> > >
> > > --
> > > Hal Finkel
> > > Postdoctoral Appointee
> > > Leadership Computing Facility
> > > Argonne National Laboratory
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
> 
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory

More information about the llvm-commits mailing list