[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: Monday, December 10, 2012 1:00:52 PM
> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in misched
> 
> 
> On Dec 10, 2012, at 10:54 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> > ----- Original Message -----
> >> From: "Andrew Trick" <atrick at apple.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: llvm-commits at cs.uiuc.edu
> >> Sent: Friday, December 7, 2012 6:16:50 PM
> >> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> >> misched
> >> 
> >> 
> >> On Dec 6, 2012, at 10:28 AM, Andrew Trick <atrick at apple.com>
> >> wrote:
> >> 
> >>> Hal,
> >>> 
> >>> I'm hoping to do some compile time measurements soon. I'll apply
> >>> your patch and verify that it isn't a problem for the usual
> >>> benchmarks. I agree that GetUnderlyingObjects should use its
> >>> MaxLookup parameter correctly. That's a more general problem that
> >>> doesn't need to hold up the patch.
> >> 
> >> I don't see any alarming compile times with your patch. Actually I
> >> tested a modified version of the patch where I removed the inner
> >> loop that revisits all found objects at each step.
> > 
> > As we've discussed, this could lead to duplicate DAG edges. As
> > you've asserted that this is not actually a correctness problem,
> > I'm certainly fine with it.
> > 
> >> Take a look at
> >> the modifications. If you agree, then please check in.
> > 
> > Thanks! r169744.
> 
> Thanks for the fix. Summarizing the code you checked in:
> 
> There is only one visited set.
> 
> +  SmallPtrSet<const Value*, 16> Visited;
> ...
> 
> Objs may contain duplicates.
> 
> +    for (SmallVector<Value *, 4>::iterator I = Objs.begin(), IE =
> Objs.end();
> +         I != IE; ++I) {
> +      V = *I;
> +      if (!Visited.insert(V))
> +        continue;
> ...
> 
> But a value is only pushed on the result list the first time it is
> marked Visited.
> 
> +      Objects.push_back(const_cast<Value *>(V));
> +    }
> +  } while (!Working.empty());
> 
> So all Objects should be unique.

You're right; I did not look carefully enough at how you had moved the Visited.insert call.

Thanks again,
Hal

> 
> -Andy
> 

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