From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7482 invoked by alias); 6 Aug 2007 16:40:02 -0000 Received: (qmail 7414 invoked by uid 22791); 6 Aug 2007 16:40:01 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 06 Aug 2007 16:39:58 +0000 Received: (qmail 16456 invoked from network); 6 Aug 2007 16:39:57 -0000 Received: from unknown (HELO gateway) (10.0.0.100) by mail.codesourcery.com with SMTP; 6 Aug 2007 16:39:57 -0000 Received: by gateway (Postfix, from userid 1010) id B956B6C0AB; Mon, 6 Aug 2007 09:39:56 -0700 (PDT) From: Richard Sandiford To: David Ung Mail-Followup-To: David Ung ,Sandra Loosemore , GCC Patches , richard@codesourcery.com Cc: Sandra Loosemore , GCC Patches Subject: Re: PATCH: MIPS 74K load/store scheduling tweak (take 2) References: <46B3C4C0.8030606@codesourcery.com> <87d4y3da61.fsf@firetop.home> <46B74A75.8070905@mips.com> Date: Mon, 06 Aug 2007 16:40:00 -0000 In-Reply-To: <46B74A75.8070905@mips.com> (David Ung's message of "Mon\, 06 Aug 2007 17\:21\:09 +0100") Message-ID: <87y7gomxb7.fsf@firetop.home> User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2007-08/txt/msg00352.txt.bz2 David Ung writes: > Richard Sandiford wrote: >> Sandra Loosemore writes: >>> + /* Conditionally swap the instructions at POS1 and POS2 in ready queue >>> + READY, also adjusting the priority of the instruction formerly at >>> + POS1 when we do so. */ >>> + >>> + static void >>> + mips_maybe_swap_ready (rtx *ready, int pos1, int pos2) >>> + { >>> + if (pos1 < pos2 >>> + && INSN_PRIORITY (ready[pos1]) + 4 >= INSN_PRIORITY (ready[pos2])) >>> + { >>> + rtx temp; >>> + INSN_PRIORITY (ready[pos1]) = INSN_PRIORITY (ready[pos2]); >>> + temp = ready[pos1]; >>> + ready[pos1] = ready[pos2]; >>> + ready[pos2] = temp; >>> + } >>> + } >> >> To be a general function rather than a 74k function, the magic value >> 4 should be a parameter too. The comment seems a bit vague: how about >> "Make sure the instruction at POS1 in ready queue READY is ahead of >> the instruction at POS2, but only if its priority is no less than >> LIMIT units of the other instruction's priority. Assume that >> only one of the instructions may issue this cycle." Copy-edit >> as necessary. >> >> It isn't obvious without the last bit why you're only swapping, >> rather than inserting POS1 directly ahead of POS2. With the >> comment there, you can remove: >> >> + /* At this point the ready queue may no longer be sorted, but that's >> + OK since 74k can't schedule concurrent load/store on the same >> + cycle. */ >> >> I'm uncertain whether setting INSN_PRIORITY is really the >> right thing to do here. David, why isn't the sorting done by >> mips_sched_reorder enough? >> > > This is the multi-issue problem. ready_sort is called more than once > during each cycle. The 1st instruction chosen during that cycle may > not be an AGEN one (eg a floating point insn or an ALU insn), which > means mips_sched_reorder2 needs to be implemented. So instead of > doing the same thing in mips_sched_reorder2, just changing the > INSN_PRIORITY gets the same effect done automatically by ready_sort. But the problem with that is that the priority sticks. If we change the priority of a load L1 (say) from A to B, the scheduler may later insert a newly-ready load L2 with a priority between A and B. We would then wrongly treat L1 as having a higher priority than L2. I think defining TARGET_SCHED_REORDER2 is the right thing. Let's move the "cycle == 0" stuff in mips_sched_reorg into Sandra's new mips_sched_init function. I think mips_sched_reorder will then be suitable for both TARGET_SCHED_REORDER and TARGET_SCHED_REORDER2. Richard