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

[Hal Finkel]

----- 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: Monday, November 19, 2012 6:01:39 PM
> Subject: RE: [llvm-commits] [PATCH] Use GetUnderlyingObjects in misched
> 
> Hal, are you planning this for 3.2?

No.

 -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