This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: PATCH: More mips3264 support


On 22 Jul 2001 11:11:40 -0700, cgd@sibyte.com wrote:
> I'm not Nick, but i'll comment anyway!  8-)
> 

How'd I guess?


> > 	* mips-dis.c: Add support for bfd_mach_mipsisa32 and
> > 	bfd_mach_mipsisa64.
> 
> puzzled about these (and the related changes elsewhere that use them).
> "MIPS32 and MIPS64 ISA" is what the existing bfd_mach_mips5 and
> bfd_mach_mips64 were meant to address.
> 
> Why are new constants needed?  Am I missing something Subtle here?
> 
> (mips64isa is a name in mips_cpu_info_table in gas only because
> historically mips64 is taken.)
> 
> As target names go, I think i'm fine with mipsisa64, though, so stuff
> related to adding that is OK by me.
> 

Yes.  Basically using MIPS32 and MIPS64 ISA are, IMO, confusing.  This
way we've separated out the base core (isa32 and isa64) for the newer
versions of the assembler/compiler.


> perhaps silly, but the order in which you've sorted the options seems
> ... wrong no matter how you look at it.  8-)
> 
> (looks good to minimize diffs & conflicts though, good for your own
> tree maintenance.  8-)
> 

;)  I'll check it to make sure.

> 
> > src/bfd/ChangeLog
> > 2001-07-19  Eric Christopher  <echristo@redhat.com>
> > 	    Jason Eckhardt  <jle@redhat.com>
> > 
> > 	* bfd/archures.c: Add mipsisa32 and mipsisa64.
> > 	* bfd/cpu-mips.c: Ditto.
> > 	* bfd/elf32-mips.c (_bfd_mips_elf_final_write_processing): Ditto.
> 
> same issue as above.
> 

I assume you meant the mipsisa32 and mipsisa64 and not the command
sorting.

> 
> > src/gas/ChangeLog
> > 2001-07-19  Eric Christopher  <echristo@redhat.com>
> > 	    Jason Eckhardt  <jle@redhat.com>
> > 
> > 	* config/tc-mips.c (mips_cpu_info): Add support for mipsisa32,
> > 	5kc, and 20kc.
> 
> if you're going to configure as mipsisaNN-whatever, don't you also
> need support in here for mipsisa64?  ("mips64isa" was an attempt to do
> that, but I can imagine it'll run afoul of more autoconf lossage than
> will mipsisa64...  if you agree, you might also nuke mips64isa.)
> 

Ok.  I'll do this and look into the patch a bit more.  Everything
_works_
but this is what peer review is for yes?

> for the MIPS32-4K cores, when reformulating this table I made it:
> 
>   { "MIPS32-4K",      0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "4kc",            0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "4km",            0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "4kp",            0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "mips32-4kc",     0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "mips32-4km",     0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>   { "mips32-4kp",     0,      ISA_MIPS32,     CPU_MIPS32_4K, },
>  
> the notion being:
> 
> 	* canonical name first (upper case, though it's compared
> 	insensitively when searching)
> 
> 	* variants that the user might resonably type, afterward.
> 
> You might want to do the same for the 5k and 20k I don't really care.
> If you do want, the entries from my (not yet submitted -- you beat me
> 8-) source tree are:
> 
>   /* MIPS64 5K CPU */
>   { "MIPS64-5K",        0,      ISA_MIPS64,     CPU_MIPS64_5K, },
>   { "5kc",              0,      ISA_MIPS64,     CPU_MIPS64_5K, },
>   { "mips64-5kc",       0,      ISA_MIPS64,     CPU_MIPS64_5K, },
>   
>   /* MIPS64 20K CPU */  
>   { "MIPS64-20K",       0,      ISA_MIPS64,     CPU_MIPS64_20K, },
>   { "20kc",             0,      ISA_MIPS64,     CPU_MIPS64_20K, },
>   { "mips64-20kc",      0,      ISA_MIPS64,     CPU_MIPS64_20K, },
> 
> 

Heh.  I'll see.

> Your current entries are a bit bogus: the table is search w/o case
> sensitivity, so there's no point in having entries that differ only by
> case.
> 

Thanks for the catch.  I'd missed that one.  

> 
> 
> Any reason for adding the blank line in opcode/mips.h?  8-)
> 
> 

Umm.. some code that didn't get deleted? ;)

> 
> and now back to the mips-opc.c changes:
> 
> > Index: opcodes/mips-opc.c
> 
> > +{"dmfc1",   "t,S,H",    0x44200000, 0xffe007f8, LCD|WR_t|RD_S|FP_S,     I64},
> 
> If you look in the MIPS32/MIPS64 manuals (e.g.
> http://www.mips.com/declassified/Declassified_2001/MD00087-2B-MIPS64BIS-AFP-00.95.pdf),
> they _do not_ define as 'sel' code for {d,}m[ft]c1!
> 
> I believe this is intentional and "correct": the FP architecture has
> the FP regs, but a selector within them has no well-defined meaning.
> 
> if you look at the descriptions of all of the mfc/mtc opcodes, you'll
> see that, alone, 1 is missing 'sel'.  (of course, you won't see c3 at
> all.  8-)
> 
> 

Hrm.  Ok, this must be a difference from the old versions and the new
versions.

> > +{"dmfc2",   "t,S,H",    0x48200000, 0xffe007f8, LCD|WR_t|RD_S|FP_S,     I64},
> 
> This one I Just Don't Get, and I have three indepent issues with this
> set of changes as is:
> 
> * You're adding interpretation of $fN as the cp register arg for c2?
> Does that make sense?  It's not the FP coprocessor.  If i _is_ an FP
> coprocessor on some machines, well, I'd say that that encoding should
> be machine-specific.  but just saying e.g. "dmfc2 $10, $f3, 0" for
> most MIPS processors doesn't make sense to me.
> 
> * you added it for dmfc2 and dmtc2, but not for mfc2 and mtc2.  That
> seems incorrect.
> 
> * you added "t,S,H" variants, but didn't add "t,S" variants.  If the
> former is appropriate, the latter must be as well I'd _think_!
> 
> 

Basically the idea behind all of this is from the original spec that we
received from MIPS.  How about I just go and assume that you did the
work
correctly when you submitted it and remove that part of the patch? ;)

Anyhow, I'll produce another set for you and Nick to comment on as soon
as I'm able to remerge all of my changes.

-eric


--
Look out behind you!


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]