[RFA:] In -build-operand!, -build-reg-operand, collect the natural mode, not the used mode,

[Doug Evans <dje at transmeta dot com>]

Sorry for the delay.  Now that I'm back up to speed on the m32r response
times may actually improve (but don't hold your breath of course, sigh).

The short answer is, please don't commit this just yet.
The purpose of this message is to (try to) solicit input from folks.

Hans-Peter Nilsson writes:
 > As the comments say and as the example show, without the patch
 > at the end of the message, more than one field with the same
 > name can appear in union sem_fields in the generated
 > sim/x/cpu.h.  For example, apply the following nonsensical but
 > supposedly harmless patch to m32r.cpu.  It makes the div insn
 > access sr in QI mode (besides the existing access in its default
 > or natural mode) and reg h-gr 13 in both SI and QI mode.  If you
 > think using the same operand in different modes in an insn is a
 > semantic error, let's fix the parser so that it's identified as
 > such.
 > 
 > Index: m32r.cpu
 > ===================================================================
 > RCS file: /cvs/src/src/cgen/cpu/m32r.cpu,v
 > retrieving revision 1.1
 > diff -c -p -u -p -r1.1 m32r.cpu
 > cvs server: conflicting specifications of output style
 > --- m32r.cpu	5 Jul 2001 12:45:47 -0000	1.1
 > +++ m32r.cpu	5 Dec 2002 19:06:16 -0000
 > @@ -1026,7 +1026,13 @@
 >       ()
 >       "div $dr,$sr"
 >       (+ OP1_9 OP2_0 dr sr (f-simm16 0))
 > -     (if (ne sr (const 0)) (set dr (div dr sr)))
 > +     (sequence
 > +       ((SI tmp))
 > +       (if (ne sr (const 0)) (set dr (div dr sr)))
 > +       (set tmp
 > +	    (add SI (ext SI (and QI (reg QI h-gr 13)
 > +				 (and QI sr 254)))
 > +		 (reg SI h-gr 13))))
 >       ((m32r/d (unit u-exec (cycles 37)))
 >        (m32rx (unit u-exec (cycles 37))))
 >  )

For the sake of discussion, let's modify this patch to be this:

diff -u -p -r1.1 m32r.cpu
--- m32r.cpu	5 Jul 2001 12:45:47 -0000	1.1
+++ m32r.cpu	13 Dec 2002 09:16:08 -0000
@@ -1026,7 +1026,14 @@
      ()
      "div $dr,$sr"
      (+ OP1_9 OP2_0 dr sr (f-simm16 0))
-     (if (ne sr (const 0)) (set dr (div dr sr)))
+     ;(if (ne sr (const 0)) (set dr (div dr sr)))
+     (sequence 
+       ((SI tmp)) 
+       (if (ne sr (const 0)) (set dr (div dr sr))) 
+       (set tmp 
+	    (add SI (ext SI (and QI (reg QI h-gr 13)
+				 (or QI (and QI sr #xf0) (and QI sr #x0f))))
+		 (reg SI h-gr 13))))
      ((m32r/d (unit u-exec (cycles 37)))
       (m32rx (unit u-exec (cycles 37))))
 )

Note the two copies of uses of `sr' in QImode.

Here's the diff I get for sim/m32r/cpu.h (without your patch):

@@ -226,9 +226,12 @@ union sem_fields {
     UINT f_r1;
     UINT f_r2;
     unsigned char in_dr;
+    unsigned char in_h_gr_QI_13;
+    unsigned char in_h_gr_SI_13;
+    unsigned char in_sr;
     unsigned char in_sr;
     unsigned char out_dr;
-  } sfmt_add;
+  } sfmt_div;
 #if WITH_SCACHE_PBB
   /* Writeback handler.  */
   struct {

Note that although `sr' is referenced twice in QImode, there's still only
one additional copy of `in_sr' (as is, still a bug of course! :-).

Note also that while there are two copies of in_h_gr_<mumble>,
there's no problem with them as the mode distinguishes them.

So another way to look at the bug (no claim is made that this
isn't obvious) is that the two copies of `in_sr' are ok except
that one or both should include a mode so there's no compilation error.
Clearly that's what's already happening with in_h_gr_<mumble>.
No claim is also made that this is a better way to look at the bug.
Maybe it is, maybe it isn't.  It comes down to "what do we want?".

We should also look at the effect on opcodes/m32r-opinst.c.
The salient change here is:

@@ -202,6 +202,16 @@ static const CGEN_OPINST sfmt_cmpz_ops[]
 
 static const CGEN_OPINST sfmt_div_ops[] = {
   { INPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
+  { INPUT, "h_gr_QI_13", HW_H_GR, CGEN_MODE_QI, 0, 13, 0 },
+  { INPUT, "h_gr_SI_13", HW_H_GR, CGEN_MODE_SI, 0, 13, 0 },
+  { INPUT, "sr", HW_H_GR, CGEN_MODE_SI, OP_ENT (SR), 0, 0 },
+  { INPUT, "sr", HW_H_GR, CGEN_MODE_QI, OP_ENT (SR), 0, 0 },
+  { OUTPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
+  { END }
+};
+
+static const CGEN_OPINST sfmt_divu_ops[] = {
+  { INPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
   { INPUT, "sr", HW_H_GR, CGEN_MODE_SI, OP_ENT (SR), 0, 0 },
   { OUTPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
   { END }

And with your patch the change to opcodes/m32r-opinst.c is:

@@ -202,6 +202,14 @@ static const CGEN_OPINST sfmt_cmpz_ops[]
 
 static const CGEN_OPINST sfmt_div_ops[] = {
   { INPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
+  { INPUT, "h_gr_SI_13", HW_H_GR, CGEN_MODE_SI, 0, 13, 0 },
+  { INPUT, "sr", HW_H_GR, CGEN_MODE_SI, OP_ENT (SR), 0, 0 },
+  { OUTPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
+  { END }
+};
+
+static const CGEN_OPINST sfmt_divu_ops[] = {
+  { INPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
   { INPUT, "sr", HW_H_GR, CGEN_MODE_SI, OP_ENT (SR), 0, 0 },
   { OUTPUT, "dr", HW_H_GR, CGEN_MODE_SI, OP_ENT (DR), 0, COND_REF },
   { END }

Is the mode relevant here?  What do you think?

IIRC, way back when, I wanted the mode to be relevant,
it was (at the time) useful information.

Plus, for architectures where the set of a register in different modes
has different effects on the register (e.g. x86, m68k),
the mode is important.  The x86 also has to deal with ah,bh,etc.
where there's no current mode one can apply to eax,ebx,etc. and get the
intended meaning.  So do we outlaw such things and say
registers can only be set in their natural mode, and for x86,m68k,etc.
we create additional registers that alias the appropriate bits of the
"real" register?  Or do we attach meaning to setting a register in
a mode different than its natural mode?
[for x86 and ah,bh,etc. we can still go the latter way and either
create new modes (blech (*1)) or define aliases]

I've started to come up with a patch that allows us to retain the mode
of the use, rather than always using the natural mode, should we want to go
this route.  I think we do.
I have one thing left to add but I've been up for a fair while
and need a break.  I'm sending this because you've been very patient
but I don't want to let this linger.

(*1) though if we ever wanted to generalize modes to handle
any portion of an object this isn't so blech (within the context
of a non-blech design/implementation :-).

 > Now, build the simulator and notice the compilation failure due
 > to this change (copyright-year change snipped) in the generated
 > code:
 > 
 > Index: cpu.h
 > ===================================================================
 > RCS file: /cvs/src/src/sim/m32r/cpu.h,v
 > retrieving revision 1.4
 > diff -c -p -r1.4 cpu.h
 > *** cpu.h	14 Nov 2001 19:51:40 -0000	1.4
 > --- cpu.h	5 Dec 2002 19:07:15 -0000
 > *************** union sem_fields {
 > *** 226,234 ****
 >       UINT f_r1;
 >       UINT f_r2;
 >       unsigned char in_dr;
 >       unsigned char in_sr;
 >       unsigned char out_dr;
 > !   } sfmt_add;
 >   #if WITH_SCACHE_PBB
 >     /* Writeback handler.  */
 >     struct {
 > --- 226,237 ----
 >       UINT f_r1;
 >       UINT f_r2;
 >       unsigned char in_dr;
 > +     unsigned char in_h_gr_QI_13;
 > +     unsigned char in_h_gr_SI_13;
 > +     unsigned char in_sr;
 >       unsigned char in_sr;
 >       unsigned char out_dr;
 > !   } sfmt_div;
 >   #if WITH_SCACHE_PBB
 >     /* Writeback handler.  */
 >     struct {
 > 
 > There are now two members called "in_sr".  Correspondingly, in
 > decode.c, "FLD (in_sr)" is (harmlessly) set twice.
 > 
 > Looking at the code (more-than-three times 8-) the mode of the
 > *use* of the operand *in the semantics* makes a difference to
 > how the operands of the insn are collected outside of the
 > semantic code.  That looks wrong: operands should be handled as
 > statically sized entities, only depending on the define-operand
 > (et al) data.  Whatever comes from compilation of the semantic
 > code should not spill over into the operand tables.
 > 
 > Maybe it'd be best to scrap the saved mode.  That'd seems best
 > handled by using another class than <operand>, which have uses
 > for the mode besides as a in-ops+out-ops container.  Then again
 > there may be use of the used size of the operand some time.
 > Also, that would add another class for that reason only, which
 > does not seem reasonable.
 > 
 > Instead I just changed to store the natural mode of the operand,
 > ignoring the used mode.  This should not matter for the
 > generated code; just that the narrowing of operands happen in
 > the operators rather than at the input.  I also did this to
 > reg-operands for consistency.
 > 
 > I built binutils and sim for m32r, xstormy16 and i960 (which BTW
 > requires a (intelligently applied s/index/o-index/g to build).
 > There were differences in the generated code.  The m32r sim
 > testsuite passed.  I'm interested in hearing what would be
 > considered a decent test-run for a change like this.
 > 
 > Ok to commit?  If another solution is suggested, I can probably
 > be lured into doing that instead.
 > 
 > 	* semantics.scm (-build-operand!, -build-reg-operand!): Store the
 > 	natural mode of the operand, not the use of it.
 > 
 > Index: semantics.scm
 > ===================================================================
 > RCS file: /cvs/src/src/cgen/semantics.scm,v
 > retrieving revision 1.1.1.1
 > diff -c -p -r1.1.1.1 semantics.scm
 > *** semantics.scm	28 Jul 2000 04:11:52 -0000	1.1.1.1
 > --- semantics.scm	5 Dec 2002 16:18:22 -0000
 > ***************
 > *** 404,412 ****
 >   
 >   (define (-build-operand! op-name op mode tstate ref-type op-list sem-attrs)
 >     ;(display (list op-name mode ref-type)) (newline) (force-output)
 > !   (let* ((mode (mode-real-name (if (eq? mode 'DFLT)
 > ! 				   (op:mode op)
 > ! 				   mode)))
 >            ; The first #f is a placeholder for the object.
 >   	 (try (list '-op- #f mode op-name #f))
 >   	 (existing-op (-rtx-find-op try op-list)))
 > --- 404,416 ----
 >   
 >   (define (-build-operand! op-name op mode tstate ref-type op-list sem-attrs)
 >     ;(display (list op-name mode ref-type)) (newline) (force-output)
 > ! 
 > !   ; For now, always use the natural mode of the operand, not of the
 > !   ; mode of the use of it.  Storing the used mode causes multiple
 > !   ; occurences of the same operand, and applications such as
 > !   ; cgen-cpu.h get confused and e.g. output structures with multiple
 > !   ; fields having the same name.
 > !   (let* ((mode (mode-real-name (op:mode op)))
 >            ; The first #f is a placeholder for the object.
 >   	 (try (list '-op- #f mode op-name #f))
 >   	 (existing-op (-rtx-find-op try op-list)))
 > ***************
 > *** 442,451 ****
 >   	 (hw (current-hw-sem-lookup-1 hw-name)))
 >   
 >       (if hw
 > ! 	; If the mode is DFLT, use the object's natural mode.
 > ! 	(let* ((mode (mode-real-name (if (eq? (rtx-mode expr) 'DFLT)
 > ! 					 (obj:name (hw-mode hw))
 > ! 					 (rtx-mode expr))))
 >   	       (indx-sel (rtx-reg-index-sel expr))
 >   	       ; #f is a place-holder for the object (filled in later)
 >   	       (try (list 'reg #f mode hw-name indx-sel))
 > --- 446,458 ----
 >   	 (hw (current-hw-sem-lookup-1 hw-name)))
 >   
 >       (if hw
 > ! 
 > ! 	; For now, always use the natural mode of the operand, not of
 > ! 	; the mode of the use of it.  Storing the used mode causes
 > ! 	; multiple occurences of the same operand, and applications
 > ! 	; such as cgen-cpu.h get confused and e.g. output structures
 > ! 	; with multiple fields having the same name.
 > ! 	(let* ((mode (mode-real-name (obj:name (hw-mode hw))))
 >   	       (indx-sel (rtx-reg-index-sel expr))
 >   	       ; #f is a place-holder for the object (filled in later)
 >   	       (try (list 'reg #f mode hw-name indx-sel))
 > 
 > brgds, H-P
 >