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

[Sergei Larin]

Hal, are you planning this for 3.2?

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