[PATCH] reenable gep merging in some constrainted cases

Xinliang David Li davidxl at google.com
Thu May 21 14:11:28 PDT 2015


On Thu, May 21, 2015 at 1:52 PM, Quentin Colombet <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?

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
>
>




More information about the llvm-commits mailing list