[llvm-commits] [PATCH] Use GetUnderlyingObjects in misched
Hal Finkel
hfinkel at anl.gov
Fri Nov 30 20:31:10 PST 2012
----- Original Message -----
> From: "Evan Cheng" <evan.cheng at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Sergei Larin" <slarin at codeaurora.org>, llvm-commits at cs.uiuc.edu
> Sent: Sunday, November 25, 2012 1:50:31 PM
> Subject: Re: [llvm-commits] [PATCH] Use GetUnderlyingObjects in misched
>
> I'm also concerned about compile time impact. Do you have some
> numbers?
Not anything comprehensive, but I'll run some tests. We certainly may want to limit the recursion depth on targets that don't see any significant benefit.
Sergei, can you quantify your observed compile-time effect?
Thanks again,
Hal
>
> Thanks,
>
> Evan
>
> On Nov 20, 2012, at 11:04 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> > ----- Original Message -----
> >> From: "Sergei Larin" <slarin at codeaurora.org>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: llvm-commits at cs.uiuc.edu, "Andrew Trick" <atrick at apple.com>
> >> Sent: Tuesday, November 20, 2012 12:25:35 PM
> >> Subject: RE: [llvm-commits] [PATCH] Use GetUnderlyingObjects in
> >> misched
> >>
> >> 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.
> >
> > Not a problem, thanks for reviewing!
> >
> > -Hal
> >
> >>
> >> 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
> >>
> >>
> >
> > --
> > Hal Finkel
> > Postdoctoral Appointee
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
--
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list