Re: [RFA] MIPS 24K Errata Patch

[Catherine Moore <clm at codesourcery dot com>]

Hi Richard,

On 04/20/2011 04:55 AM, Richard Sandiford wrote:
> Catherine Moore<clm@codesourcery.com>  writes:
> > On 04/19/2011 03:41 PM, Richard Sandiford wrote:
> > > Catherine Moore<clm@codesourcery.com>   writes:
> > > > I've attached updated patches for the testsuite and the assembler.  I
> > > > think that I've addressed all of your comments.  Do these new patches
> > > > now look okay?
> > >
> > > Thanks for the updates.  They look good apart from a few minor
> > > stylistic nits.  I'm still not sure about the last point though:
> > >
> > > [ Hmm, I see I got the hex offsets wrong last time, sorry.  I realise
> > >     it probably didn't make much sense with those mistakes. ]
> > >
> > >    The last case is interesting.  Does the errata still trigger if
> > >    a suxc1 instruction straddles a cache boundary?  E.g., playing
> > >    battleships for a moment:
> > >
> > >        0 2 4 6
> > >      0 ........
> > >      8 ........
> > >     10 ........
> > >     18 ....XXXX   suxc1 to 0x1C
> > >     20 XXXX....
> > >     28 ........
> > >     30 ..XX....   sh to 0x32
> > >     38 ......XX   sh to 0x3E
> > >
> > >    It looks like the range checks will return 0 in this case, even though
> > >    all three stores write to different doublewords of the second cache line.
> >
> > Is there any instruction besides suxc1 that is of concern (ie.  any
> > other instruction that allows for unaligned access)?  I've altered the
> > original patch to insert nops for the case where the offset field is a
> > register.  suxc1 is an instruction in this category.
>
> Yeah, sorry for the noise.  As is so often the case, it only occured to
> me after signing off that this was now handled earlier.

No problem.  Glad we got it sorted out.


> > !   	      /* Don't swap if -mfix-24k and previous insn is a store.  */
> > !   	      || (mips_fix_24k
> > !  		&&  (prev_pinfo&  INSN_STORE_MEMORY)))
>
> This shouldn't be necessary.  It ought to be caught by:
>
> 	      /* Check for conflicts between the swapped sequence and the
> 		 target of the branch.  */
> 	      || nops_for_sequence (2, history + 1, ip, history)>  0
>
> The patch is OK without this hunk, but if you think it's needed,
> let me know.
I've removed this, but it required that the 24k-branch-delay test be 
updated.  I agree that it's not necessary.

I've attached the patches that I committed for reference.

Thank you for the thorough review.

Catherine

2011-04-20  Catherine Moore  <clm@codesourcery.com>
            David Ung <davidu@mips.com>

	* config/mips.c (mips_cl_insn): Add new field complete_p.
	(create_insn): Initialize complete_p to zero.
	(BASE_REG_EQ): New.
	(fix_24k_align_to): New.
	(fix_24k_store_info): Declare.
	(fix_24k_sort): New.
	(fix_24k_record_store_info): New.
	(nops_for_24k): New.
	(nops_for_insn): Call nops_for_24k.
	(append_insn): Move O_constant expression handling.

2011-04-20  Catherine Moore  <clm@codesourcery.com>
            David Ung <davidu@mips.com>

	* gas/mips/24k-branch-delay-1.d: New.
	* gas/mips/24k-branch-delay-1.s: New.
	* gas/mips/24k-triple-stores-1.d: New.
	* gas/mips/24k-triple-stores-1.s: New.
	* gas/mips/24k-triple-stores-2.d: New.
	* gas/mips/24k-triple-stores-2.s: New.
	* gas/mips/24k-triple-stores-3.d: New.
	* gas/mips/24k-triple-stores-3.s: New.
	* gas/mips/24k-triple-stores-4.s: New.
	* gas/mips/24k-triple-stores-4.d: New.
	* gas/mips/24k-triple-stores-5.d: New.
	* gas/mips/24k-triple-stores-5.s: New.
	* gas/mips/24k-triple-stores-6.d: New.
	* gas/mips/24k-triple-stores-6.s: New.
	* gas/mips/24k-triple-stores-7.d: New.
	* gas/mips/24k-triple-stores-7.s: New.
	* gas/mips/24k-triple-stores-8.d: New.
	* gas/mips/24k-triple-stores-8.s: New.
	* gas/mips/24k-triple-stores-9.d: New.
	* gas/mips/24k-triple-stores-9.s: New.
	* gas/mips/24k-triple-stores-10.d: New.
	* gas/mips/24k-triple-stores-10.s: New.
	* gas/mips/24k-triple-stores-11.d: New.
	* gas/mips/24k-triple-stores-11.s: New.
	* gas/mips/mips.exp: Invoke new tests.

Attachment: 24k.patch-rev3
Description: Text document

Attachment: 24k-testsuite.patch-rev3
Description: Text document