[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>, "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