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

[Evan Cheng]

I'm also concerned about compile time impact. Do you have some numbers?

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