Re: Patch: add prefix to condition args in opcodes

["Jerry Quinn" <jquinn@nortelnetworks.com>]

 >   In message <199907092055.NAA11875@cygnus.com>you write:
 >   > Because all the conditions are in a prefix, I just added extra codes to
 >   > deal with various 64 bit conditions.  You'll see that there are some
 >   > stubs for different new conditions that aren't filled in yet.

 > This should have been a completely separate patch.  There is *no* reason
 > to include this stuff as part of the basic reorganization.  And by doing so
 > all you end up doing is making more work for me as I try to sort out if any
 > of the PA2.0 stuff is actually correct.
 > 

Sorry.  I was thinking they were close enough, but you're right.  They are
independent.

 >   > Let me know what you think.  If this is OK, I suggest we prefix the float
 >   > args next.
 > I'm happy with the general concept, but the code itself needs some work.

I think we should also do the completers and float regs once you're happy with 
this change.

 > First, you reversed the patch.  ie, you did diff new old.  Not a major
 > problem, but it did take a few seconds to realize what had happened (I kept
 > looking for something nested inside the '?' case in the "new" code and
 > couldn't find it).

 > Anyway the code itself.
 > 
 > 		       /* Handle an add condition.  */
 > 		     case 'A':     /* 64 bit add */
 > 		       if (*s == ',' && *(s + 1) == '*')
 > 			 {
 > 			   cond_64 = 1;
 > 			   s += 2; /* Eat up the ,* */
 > 			 }
 > 		       else break;
 > 

[style comments snipped]

I'll change those around.  It's just the habits I'm used to.

  
 > I'm not particular happy with the way 'A' falls into 'a'.  It may be cleaner
 > to do
 > 
 > 	   case 'A':
 > 	   case 'a':
 > 	     if (*args == 'A')
 > 	       {
 > 		  Do the few "A" specific things
 > 	       }
 > 	     Do generic code for 'A' and 'a'.
 > 
 > The flow control (at least to me) is easier to follow with that kind of style
 > (it's still not ideal, but I'm not sure what else we can do to clean this up
 > without starting over with a complete redesign).

I saw that construction and I had thought what I did was preferable because it 
avoided a redundant test.  If you think it's better to write it that way, I'll 
change it.


 > These lines in the 'a' handling are indented improperly:

 > 		     opcode |= cmpltr << 13;
 > 		     INSERT_FIELD_AND_CONTINUE (opcode, flag, 12);

 > They need to be moved two columns to the right.  Similar problems occur with
 > the '@' handling.

 > In the 'b' case the INSERT_FIELD_AND_CONTINUE needs to be moved to the left
 > two columns.

OK, I missed these when going back and reformatting.

 > Do not write "abort()", instead write "abort ()".  Seems like a nit, but we
 > need to try and be consistent with the coding standards.

 > You have some comments that wrap when viewed on an 80 column screen.  They
 > need to be cleaned up.

Done.

 > This code in 'b' does not work if we had previously come in from the 'B'
 > case and had found a 64bit completer completer.  *s will have already been
 > advanced past the ,*:

 >                     cmpltr = 0;
 >                     if (*s == ',')
 >                       {
 >                         s++;

Looking at my code, I realized I fixed this but forgot to put it into the
patch.  It'll go into the revamped patches.

 > I'm not sure the disassembler changes are right, particularly in how they
 > handle the 64bit stuff.  But then again,  I would claim we shouldn't be
 > trying to add the 64bit stuff as part of this patch anyway.

I'll take another look at it and see if I'm missing anything there.

 > Please break out the basic reorganization from the 64bit stuff.  Let's get
 > the reorgs done and installed, then we can add the 64bit stuff separately.

OK.  I'll go back and redo this into two patches.  I'll make sure the
completer prefix patch is cleaned up as well.

Welcome back.  Glad to see you now have a chance to breathe :-)

Jerry

-- 
Jerry Quinn                             Tel: (514) 761-8737
jquinn@nortelnetworks.com               Fax: (514) 761-8505
Speech Recognition Research