[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?

[Philip Reames]

On 07/16/2015 06:04 PM, Daniel Berlin wrote:
> That should be literally impossible, which makes me think something
> was tested wrong
>
> The second patch i posted disables scalar pre (assuming you use
> -disable-pre) but not load pre.
>
> Since the patch you reverted touched only scalar pre, disabling scalar
> pre should *also* do the same thing.
Unless that benchmark is relying on *some* scalar PRE happening, but not 
"too much".  i.e. it could be a really really fragile benchmark which 
seems to fit the description.
>
>
> On Thu, Jul 16, 2015 at 5:43 PM, Lawrence <lawrence at codeaurora.org> wrote:
>> Hi, Daniel:
>>
>> Something interesting, even though your patch performed better for our precheckin perf run, it doesn't help the test I was looking at,  for my case, revert your patch give the best results.
>>
>> I will revert internally for now, and looking for better solution in the future.
>>
>> Regards
>>
>> Lawrence Hu
>>
>>
>> -----Original Message-----
>> From: Lawrence [mailto:lawrence at codeaurora.org]
>> Sent: Wednesday, July 15, 2015 9:36 PM
>> To: 'Daniel Berlin'
>> Cc: 'LLVM Developers Mailing List'
>> Subject: RE: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
>>
>> Hi, Daniel:
>>
>> Thanks, I tried that patch you provided, it is better than just disabling your previous patch, it has more improvements than degradations.
>>
>> Do you want to post that patch or you want me to do that?
>>
>> Regards
>>
>> Lawrence Hu
>>
>> -----Original Message-----
>> From: Daniel Berlin [mailto:dberlin at dberlin.org]
>> Sent: Wednesday, July 15, 2015 1:36 PM
>> To: Lawrence
>> Cc: LLVM Developers Mailing List
>> Subject: Re: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
>>
>> On Wed, Jul 15, 2015 at 1:10 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>> IMHO, This doesn't make a lot of sense to turn off this part on it's own.
>>> I would just use the enable-pre flag to turn off scalar PRE, as it
>>> will cause the same issue in other cases as well.
>>> Is there some reason you aren't just doing that?
>>> I suspect if this is a performance win, that would be as well.
>>>
>> Ugh, actually, it should be a win with the following change:
>>
>>
>> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp index 2c47a8a..a3387e3 100644
>> --- a/lib/Transforms/Scalar/GVN.cpp
>> +++ b/lib/Transforms/Scalar/GVN.cpp
>> @@ -1767,7 +1767,7 @@ bool GVN::processNonLocalLoad(LoadInst *LI) {
>>     }
>>
>>     // Step 4: Eliminate partial redundancy.
>> -  if (!EnablePRE || !EnableLoadPRE)
>> +  if (!EnableLoadPRE)
>>       return false;
>>
>>     return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
>>
>>
>>
>>
>> This will disable Scalar PRE without disabling load PRE.
>>
>>
>> (note, again, however, that load PRE can create exactly the same GEP situation you are referring to)
>>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev