This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFA] MIPS 24K Errata Patch
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.
The patch is OK with:
> + /* Return the minimum alignment for this store instruction. */
> +
> + static int
> + fix_24k_align_to (const struct mips_opcode *mo)
> + {
> +
Remove the last blank line. There's supposed to be a blank line
after the comment, but not after the "{". Same throughout.
> + struct store_info
Although this structure is general, the functions that manipulate it aren't.
Let's call it fix_24k_store_info for now, and we can generalise it if we
need it for something else.
> + /* Setup store information to be used by nops_for_24k. */
> +
> + static void
> + setup_24k_position_info (struct store_info *stinfo, const struct mips_cl_insn *insn)
> + {
Long line. Format as:
setup_24k_position_info (struct store_info *stinfo,
const struct mips_cl_insn *insn)
{
I think it would be better to return a bool that says whether the extraction
is possible. Something like:
/* INSN is a store instruction. Try to record the store information
in STINFO. Return false if the information isn't known. */
static bfd_boolean
fix_24k_record_store_info (struct store_info *stinfo,
const struct mips_cl_insn *insn)
{
/* The instruction must have a known offset. */
if (!insn->complete || !strstr (insn->insn_mo->args, "o("))
return FALSE;
stinfo->off = (insn->insn_opcode >> OP_SH_IMMEDIATE) & OP_MASK_IMMEDIATE;
stinfo->align_to = fix_24k_align_to (insn->insn_mo);
return TRUE;
}
Then:
> + setup_24k_position_info (&pos[0], insn);
> + setup_24k_position_info (&pos[1], &hist[0]);
> + setup_24k_position_info (&pos[2], &hist[1]);
> +
> + /* If we don't know the offset, then assume overlap. */
> + if (pos[0].register_offset == TRUE
> + || pos[1].register_offset == TRUE
> + || pos[2].register_offset == TRUE)
> + return 1;
> +
> + if (insn->complete_p == 0
> + || history[0].complete_p == 0
> + || history[1].complete_p == 0)
> + return 1;
becomes:
if (!fix_24k_record_store_info (&pos[0], insn)
|| !fix_24k_record_store_info (&pos[1], &hist[0])
|| !fix_24k_record_store_info (&pos[2], &hist[1]))
return 1;
> + running in 1:2 mode, it is not possible to create the sequence above
> + using only floating point store instructions.
> +
> + In this case, the cache line being filled is incorrectly marked
> + invalid, thereby losing the data from any store to the line that
> + occurs between the original miss and the completion of the five
> + cycle sequence shown above.
The last paragraph is indented one character too far.
> + if (mips_fix_24k)
> + {
> + tmp_nops = nops_for_24k (hist, insn);
> + if (tmp_nops > nops)
> + nops = tmp_nops;
> + }
> +
> +
Drop the last blank line here too.
> ! if (ip->complete_p == 0
> ! && *reloc_type < BFD_RELOC_UNUSED)
!ip->complete_p rather than ip->complete_p == 0
> ! /* 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.
Looks good otherwise. Thanks for your patience!
Richard