On Oct 22, 2014, at 9:24 AM, Richard Sandiford <richard.sandif...@arm.com> wrote:
> Maxim Kuvyrkov <maxim.kuvyr...@linaro.org> writes: >> On Oct 21, 2014, at 9:11 PM, Richard Sandiford >> <richard.sandif...@arm.com> wrote: >> >>> Maxim Kuvyrkov <maxim.kuvyr...@linaro.org> writes: >>>> This patch improves model_order_p to use non-reg-pressure version of >>>> rank_for_schedule when it needs to break the tie. At the moment it is >>>> comparing INSN_PRIORITY by itself, and it seems prudent to outsource >>>> that to rank_for_schedule. >>> >>> Do you have an example of where this helps? A possible danger is that >>> rank_for_schedule might (indirectly) depend on state that isn't maintained >>> or updated in the same way during the model schedule phase. >> >> I don't have an example where this patch helps, and I consider this >> patch a general cleanup. From what I can see, all scheduler data >> structures are maintained during reg_sched_model scheduling. > > I'm not really sure it's a cleanup though. The code isn't aesthetically > cleaner because of the way it has to switch the global sched_pressure > variable for the duration of the call. And it doesn't really seem > conceptually cleaner. The model ordering was deliberately simple > because the model schedule doesn't take pipeline characteristics like > issue delays or unit conflicts into account; all it's doing is trying to > minimise the register pressure. It isn't obvious to me that all the > extra stuff that rank_for_schedule does would make sense in that context > (i.e. that it would tend to reduce pressure compared to the current test, > rather than increase it). It also looks like using rank_for_schedule would > increase compile time. > > So personally I'd rather keep things the way they are unless we have > a motivating example. I will not be pushing for this particular change, I'm fine with dropping the patch if current code looks more aesthetically pleasing. Thanks, -- Maxim Kuvyrkov www.linaro.org
