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

[Sergei Larin]

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.

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