Re: MIPS32 / MIPS64 release 2 support in simulator

[cgd@broadcom.com]

[-- Attachment #1: Type: text/plain, Size: 2748 bytes --]

Andrew (or somebody), could you please confirm that their assignment
is in place?  (I don't have access to anything that would let me check
easily, as far as I know.)


At Wed, 3 Nov 2004 15:26:27 +0000 (UTC), "David Ung" wrote:
> The following file is for MIPS32 / MIPS64 release 2 extensions to the
> simulator.

Based on the way other bits are done:

* these should be placed in mips.igen proper (since they're part of a
  standard MIPS ISA level).  If you'd like to try dissuade me from
  this position, I'm willing to discuss it.  But right now, all
  *other* standard MIPS ISA bits are in mips.igen itself.

* the lines like:

        *mips32,mips64:

  should be two separate lines.


Also:

* If you can't provide diffs for new files, at least provide them as
  an attachment.  (If you're interested in faking out CVS so you can
  provide the diffs easily, see the attached script.)  Otherwise, I
  have fears about whitespace and/or line wrapping.

* If you do convince me that a new .igen file is appropriate, you'll
  need to put an appropriate copyright notice and license on it.
  I'd suggest starting with the one on, say, mips3d.igen.

* You need testsuite coverage for all of the new instructions, please.

* Please carefully review the instruction implementations against the
  manuals, and against the defines and functions already provided by
  the simulator.  Right now, there are some issues.

  For instance:

        * DI, EI should sign extend the result, and should use
          status_IE rather than '1'.

        * I believe you should be using TRACE_ALU_INPUT0 in DI, EI and
          in possibly similar places.

        * you are missing letters in wrpgpr and rdpgpr in comment.

        * in dshd, I get what you're trying to do, but the code should
          be less complex to do it.  (in particular, the use of the
          0xffff0000 masks is confusing).  Just write the code the
          obvious way.  8-)

        * should use check_fpu in FPU ops, and I believe you should
          be using check_u64 in the 64-bit ops.  Also, appropriate use
          of NotWordValue would be good.

        * should probably just move the ror/rorv instructions from
          vr.igen into mips.igen, and use them since they are as far
          as I know otherwise the same.  Yes, I know they don't print
          out w/ the official MIPS mnemonics, but the assembler
          recognizes both and, well, uh, MIPS named them last.  8-)

    There may be more, those are just the things I noticed on a quick
    read-through.  (Obviously, I'll have to review any updated patch.)


(We did an implementation of r2, too, but I've never had time to
submit it back.  *sigh*  Now *I* get to lose on the merge.  8-)



chris


[-- Attachment #2: fakeadd --]
[-- Type: application/octet-stream, Size: 666 bytes --]

#!/bin/sh

if [ $# -eq 0 ]; then
	echo "usage: $0 file ..." 1>&2
	exit 1
fi

if [ -d CVS ]; then :; else
	echo "$0: fatal error: no CVS directory!
	exit 1
fi
if [ -f CVS/Entries ] && [ -w CVS/Entries ]; then :; else
	echo "$0: fatal error: CVS/Entries file not writable!
	exit 1
fi

rv=0
for file in "$@"; do
	if [ -f "$file" ]; then :; else
		echo "$0: error: \"$file\" does not exist or is not a plain file"
		rv=1
		continue
	fi

	if grep "^/$file/" CVS/Entries > /dev/null 2>&1; then
		echo "$0: error: \"$file\" is already listed in CVS/Entries"
		rv=1
		continue
	fi

	echo "/$file/0/dummy timestamp//" >> CVS/Entries
	echo "$0: added \"$file\""
done
exit $rv