Ping. Could x86 maintainer(s) look at these changes? Thanks, Igor
On Fri, Apr 20, 2012 at 4:04 PM, Igor Zamyatin <izamya...@gmail.com> wrote: > On Tue, Apr 17, 2012 at 12:27 AM, Igor Zamyatin <izamya...@gmail.com> wrote: >> On Fri, Apr 13, 2012 at 4:20 PM, Andrey Belevantsev <a...@ispras.ru> wrote: >>> On 13.04.2012 14:18, Igor Zamyatin wrote: >>>> >>>> On Thu, Apr 12, 2012 at 5:01 PM, Andrey Belevantsev<a...@ispras.ru> >>>> wrote: >>>>> >>>>> On 12.04.2012 16:38, Richard Guenther wrote: >>>>>> >>>>>> >>>>>> On Thu, Apr 12, 2012 at 2:36 PM, Igor Zamyatin<izamya...@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On Thu, Apr 12, 2012 at 4:24 PM, Richard Guenther >>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Apr 12, 2012 at 2:00 PM, Alexander Monakov<amona...@ispras.ru> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> Can atom execute two IMUL in parallel? Or what exactly is the >>>>>>>>>> pipeline >>>>>>>>>> behavior? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> As I understand from Intel's optimization reference manual, the >>>>>>>>> behavior is as >>>>>>>>> follows: if the instruction immediately following IMUL has shorter >>>>>>>>> latency, >>>>>>>>> execution is stalled for 4 cycles (which is IMUL's latency); >>>>>>>>> otherwise, >>>>>>>>> a >>>>>>>>> 4-or-more cycles latency instruction can be issued after IMUL without >>>>>>>>> a >>>>>>>>> stall. >>>>>>>>> In other words, IMUL is pipelined with respect to other long-latency >>>>>>>>> instructions, but not to short-latency instructions. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> It seems to be modeled in the pipeline description though: >>>>>>>> >>>>>>>> ;;; imul insn has 5 cycles latency >>>>>>>> (define_reservation "atom-imul-32" >>>>>>>> "atom-imul-1, atom-imul-2, atom-imul-3, >>>>>>>> atom-imul-4, >>>>>>>> atom-port-0") >>>>>>>> >>>>>>>> ;;; imul instruction excludes other non-FP instructions. >>>>>>>> (exclusion_set "atom-eu-0, atom-eu-1" >>>>>>>> "atom-imul-1, atom-imul-2, atom-imul-3, atom-imul-4") >>>>>>>> >>>>>>> >>>>>>> The main idea is quite simple: >>>>>>> >>>>>>> If we are going to schedule IMUL instruction (it is on the top of >>>>>>> ready list) we try to find out producer of other (independent) IMUL >>>>>>> instruction that is in ready list too. The goal is try to schedule >>>>>>> such a producer to get another IMUL in ready list and get scheduling >>>>>>> of 2 successive IMUL instructions. >>>>>> >>>>>> >>>>>> >>>>>> Why does that not happen without your patch? Does it never happen >>>>>> without >>>>>> your patch or does it merely not happen for one EEMBC benchmark (can >>>>>> you provide a testcase?)? >>>>> >>>>> >>>>> >>>>> It does not happen because the scheduler by itself does not do such >>>>> specific >>>>> reordering. That said, it is easy to imagine the cases where this patch >>>>> will make things worse rather than better. >>>>> >>>>> Igor, why not try different subtler mechanisms like adjust_priority, >>>>> which >>>>> is get called when an insn is added to the ready list? E.g. increase the >>>>> producer's priority. >>>>> >>>>> The patch as is misses checks for NONDEBUG_INSN_P. Also, why bail out >>>>> when >>>>> you have more than one imul in the ready list? Don't you want to bump >>>>> the >>>>> priority of the other imul found? >>>> >>>> >>>> Could you provide some examples when this patch would harm the >>>> performance? >>> >>> >>> I thought of the cases when the other ready insns can fill up the hole and >>> that would be more beneficial because e.g. they would be on more critical >>> paths than the producer of your second imul. I don't know enough of Atom to >>> give an example -- maybe some long divisions? >>> >>> >>>> >>>> Sched_reorder was chosen since it is used in other ports and looks >>>> most suitable for such case, e.g. it provides access to the whole >>>> ready list. >>>> BTW, just increasing producer's priority seems to be more risky in >>>> performance sense - we can incorrectly start delaying some >>>> instructions. >>> >>> >>> Yes, but exactly because of the above example you can start incorrectly >>> delaying other insns, too, as you force the insn to be the first in the >>> list. While bumping priority still leaves the scheduler sorting heuristics >>> in place and actually lowers that risk. >>> >>>> >>>> Thought ready list doesn't contain DEBUG_INSN... Is it so? If it >>>> contains them - this could be added easily >>> >>> >>> It does, but I'm not sure the sched_reorder hook gets them or they are >>> immediately removed -- I saw similar checks in one of the targets' hooks. >> >> Done with DEBUG_INSN, also 1-imul limit was removed. Patch attached >> >>> >>> Anyways, my main thought was that it is better to test on more benchmarks to >>> alleviate the above concerns, so as long as the i386 maintainers are happy, >>> I don't see major problems here. A good idea could be to generalize the >>> patch to handle other long latency insns as second consumers, not only >>> imuls, if this is relevant for Atom. >> >> Yes, generalization of this approach is in plans. According to Atom >> Software optimization guide there are several headrooms left here. >> As for trying on more benchmarks - the particular case is indeed quite >> rare. I attached the example >> where patch helps to group imuls in pairs which is profitable for >> Atom. Such and similar codes are not very common. >> But hopefully this approach could help avoid this and other glassjaws. > > BTW, this patch also helps some EEMBC tests when funroll-loops specified. > So, any feedback from i386 maintainers about this? :) > > Changelog slightly changed > > > 2012-04-10 Yuri Rumyantsev <yuri.s.rumyant...@intel.com> > > * config/i386/i386.c (x86_sched_reorder): New function. > Added new function x86_sched_reorder > to take advantage of Atom pipelened IMUL execution. > > >> >>> >>> Andrey >>> >>> >>>> >>>> The case with more than one imul in the ready list wasn't considered >>>> just because the goal was to catch the particular case when there is a >>>> risk to get the following picture: "imul-producer-imul" which is less >>>> effective than "producer-imul-imul" for Atom >>>> >>>>> >>>>> Andrey >>>>> >>>>> >>>>>> >>>>>>> And MD allows us only prefer scheduling of successive IMUL >>>>>>> instructions, >>>>>>> i.e. >>>>>>> If IMUL was just scheduled and ready list contains another IMUL >>>>>>> instruction then it will be chosen as the best candidate for >>>>>>> scheduling. >>>>>>> >>>>>>> >>>>>>>> at least from my very limited guessing of what the above does. So, >>>>>>>> did >>>>>>>> you >>>>>>>> analyze why the scheduler does not properly schedule things for you? >>>>>>>> >>>>>>>> Richard. >>>>>>>> >>>>>>>>> >>>>>>>>> From reading the patch, I could not understand the link between >>>>>>>>> pipeline >>>>>>>>> behavior and what the patch appears to do. >>>>>>>>> >>>>>>>>> Hope that helps. >>>>>>>>> >>>>>>>>> Alexander >>>>> >>>>> >>>>> >>>> >>>> Thanks, >>>> Igor >>> >>>
