[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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: misched_objs-v2.patch
Type: text/x-patch
Size: 13147 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121118/4d601ca2/attachment.bin>