On Wed, May 15, 2019 at 5:51 PM Kewen.Lin <li...@linux.ibm.com> wrote: > > on 2019/5/15 下午4:50, Bin.Cheng wrote: > > On Wed, May 15, 2019 at 4:38 PM Kewen.Lin <li...@linux.ibm.com> wrote: > >> > >> on 2019/5/15 上午10:11, Kewen.Lin wrote: > >>> > >>> on 2019/5/14 下午3:18, Richard Biener wrote: > >>>> Hum. The function is somewhat of a hack, trying to produce > >>>> "reasonable" DECL_RTL, exposing it in expr.[ch] with this > >>>> name is eventually misleading. Also you fail to "outline" > >>>> the most important part: > >>>> > >>>> FOR_EACH_VEC_ELT (decl_rtl_to_reset, i, obj) > >>>> SET_DECL_RTL (obj, NULL_RTX); > >>>> > >>>> which IMHO would warrant making this machinery a class > >>>> with the above done in its destructor? > >>>> > >>> > >>> Good suggestion! In the IVOPTS implementation, it has one > >>> interface free_loop_data to clean up some data structures > >>> including this decl_rtl_to_reset. While for the use in > >>> "PATCH v2 2/3", we have to clean it artificially once > >>> expanding finishes. > >>> > >>> It's better to make it as a class and ensure the clean of > >>> the vector in its destructor. > >>> > >> > >> Hi Bin, > >> > >> Could you kindly comment this? For the use in "PATCH v2 2/3", > >> it's still the same with destructor change. But for the use > >> in IVOPTs, I don't know the historical reason why to clean > >> up in free_loop_data instead of in place? If it's possible to > > This is quite old code in ivopts, I suppose it's for caching results > > as you guessed. I didn't measure if how much the caching affects > > performance. It can be simply removed if not as useful as expected. > > Of course we need experiment data here. > > Got it, thanks! > > > > > Please correct me if I am wrong. This is factored out of ivopts in > > order to reuse it in the new target hook? As discussed in another > > thread, most target hook code can be moved to ivopts, so this > > refactoring and in-place freeing would be unnecessary, and the new > > code can also take advantage of caching? > > > > Yes, you are absolutely right. If we move the generic part ( > especially costly_iter_for_doloop_p) to IVOPTs, the factoring isn't > necessary any more. > > I thought it may be still nice to refactor it even if we will go > with that moving of generic part. But the code exists for a long > time but there is no requests to factor it out all the time, it > looks useless to expose it speculatively? Just do it if you think necessary. The criteria is simple: whether it results in better code. You don't need any approval to improve GCC. :-)
Thanks, bin > > OK, I'll cancel factoring it then. > > > Thanks, > Kewen > > > Thanks, > > bin > > > >> reuse, for example some object was prepared and we don't need > >> to prepare for it again during the same loop handling, then > >> the destructor proposal will clean it up too early and > >> inefficient. If so, one flag in constructor to control this > >> might be required. > >> > >> Thanks a lot in advance! > >> > >> Kewen > >> > >> >
