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

[Hal Finkel]

----- 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.
> 
> +    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.
> 
> +      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.
> 
>      } 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. It looks like you do the right thing for loads already.

Thanks! I'll prepare an updated patch.

Also, is it possible for a PseudoSourceValue to alias with an "identified object"? The code implicitly assumes that this is true, but I think that is too pessimistic.

Thanks again,
Hal

> 
> -Andy
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory