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

[Sergei Larin]

Hal,

  Your second version of the patch does not seem to contain the test case anymore - was it omitted temporarily for reviewing purposes and will be present in the final patch? ... or is it irrelevant after the first review fixes?

Also here:

static void getUnderlyingObjectsForInstr(const MachineInstr *MI,
              const MachineFrameInfo *MFI,
              SmallVectorImpl<std::pair<const Value *, bool> > &Objects) {
  if (!MI->hasOneMemOperand() ||
      !(*MI->memoperands_begin())->getValue() ||
      (*MI->memoperands_begin())->isVolatile())
    return;

  const Value *V = (*MI->memoperands_begin())->getValue();
  if (!V)
    return;

Can you have multiple memory operands in an MI for PPC? I know it is beyond the scope of this patch, but I simply wonder since we are on it.

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