[PATCH] reenable gep merging in some constrainted cases

[Quentin Colombet]

> On May 21, 2015, at 2:11 PM, Xinliang David Li <davidxl at google.com> wrote:
> 
> On Thu, May 21, 2015 at 1:52 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> Hi David,
>> 
>> On May 19, 2015, at 3:26 PM, Xinliang David Li <xinliangli at gmail.com> wrote:
>> 
>> I agree. I remembered the plan was to disable the blind GEP merging which
>> have shown big damages to performance, reported independently by different
>> users, and tune the  heuristic to enable beneficial merging selectively.
>> 
>> 
>> My concern is that we will end up tuning a heuristic before having really
>> think about our alternative. Since we are still in the early stage of this,
>> I was wondering if it wouldn’t be best to step back and have a better
>> understanding of our options.
>> 
>> My initial approval for the plan was based on the fact that we did not see
>> any actual regressions, but it seems it was not the case.
> 
> Quentin, I suggest putting the disabling GEP merging and related
> heuristic tuning under an internal option for now.  With this option
> in place, we can easily flip it on/off in analyzing specific
> issues/regressions and verifying if gep merging is related. Since this
> is the only regression found, I think we should also leave disabling
> GEP merging as the default for now. If we end up finding more than 2
> or 3 different regressions due to this being on, we can re-visit the
> solution with all collected examples and perhaps flip the default.
> 
> What do you think?

SGTM.

Thanks,
Q.

> 
> thanks,
> 
> David
> 
> 
>> 
>> Thoughts?
>> 
>> Cheers,
>> -Quentin
>> 
>> 
>> David
>> 
>> 
>> On Tue, May 19, 2015 at 3:10 PM, Wei Mi <wmi at google.com> wrote:
>>> 
>>> Hi Quentin,
>>> 
>>> Yes, it is a tuning of the heuristic from http://reviews.llvm.org/D8911.
>>> 
>>> Because the regression I saw here has much smaller negative impact
>>> compared with the original negative impact of gep merging, is it
>>> possible that we leave r235455 there while we discuss a better fix?
>>> Because in this way we can check is there any other analysis affected
>>> by disabling gep merging.
>>> 
>>> Thanks,
>>> Wei.
>>> 
>>> On Tue, May 19, 2015 at 3:00 PM, Quentin Colombet <qcolombet at apple.com>
>>> wrote:
>>>> Hi Wei,
>>>> 
>>>> Correct me if I am wrong, but this is a tuning of the heuristic from
>>>> http://reviews.llvm.org/D8911, right?
>>>> 
>>>> Given that you actually see a regression now (and the fact that after
>>>> discussing with Chandler, I am not convinced the initial change makes
>>>> sense), I would suggest that you revert r235455 and that we start again the
>>>> discussion on how to properly fix that.
>>>> 
>>>> Thanks,
>>>> -Quentin
>>>> 
>>>> 
>>>> REPOSITORY
>>>>  rL LLVM
>>>> 
>>>> http://reviews.llvm.org/D9865
>>>> 
>>>> EMAIL PREFERENCES
>>>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150521/baf8901d/attachment.html>