Re: msp430 port

[DJ Delorie <dj at redhat dot com>]

[nickc added for comments about the bits he wrote]

> ... define these as
> 
> (define_predicate "msp_general_operand"
>   (match_code "mem,reg,subreg,const_int,const,symbol_ref,label_ref"
> {
>   int save_volatile_ok = volatile_ok;
>   volatile_ok = 1;
>   int ret = general_operand (op, mode);
>   volatile_ok = save_volatile_ok;
>   return ret;
> })
> 
> That said, why do they exist?

Because gcc refuses to optimize operations with volatile memory
references, even when the target has opcodes that follow the volatile
memory rules.  "set bit in memory" for example.  I've had to do
something like this for every port I've written, for the same reason,
despite arguing against gcc's pedantry about volatile memory accesses.

> > +;; The next two patterns are here to support a "feature" of how GCC implements
> > +;; varargs.  When a function uses varargs and the *second* to last named
> > +;; argument is split between argument registers and the stack, gcc expects the
> > +;; callee to allocate space on the stack that can contain the register-based
> > +;; part of the argument.  This space *has* to be just before the remaining
> > +;; arguments (ie the ones that are fully on the stack).
> 
> Ug.  We ought to have been able to convince the compiler to copy the other
> direction.  I.e. create storage for that argument as if it were a regular local
> variable, spill the register portion into it, and also copy in the fragment
> from the stack.
> 
> That sort of solution seems better than fiddling the location of the return
> address in both directions.

Nick, this one is yours.

> No constraints on splits.
> 
> Nor here.

Fixed.  Perhaps gen* could error on those?  I know they're ignored, I
just keep forgetting that splits are special.

(alternately, why can't they be included in the matching logic?)

> > +;; Alternatives 2 and 3 are to handle cases generated by reload.
> > +(define_insn "subpsi3"
> > +  [(set (match_operand:PSI            0 "nonimmediate_operand" "=r,   rm, &?r, ?&r")
> > +	(minus:PSI (match_operand:PSI 1 "general_operand"       "0,   0,   !r,  !i")
> > +		   (match_operand:PSI 2 "general_operand"       "rLs, rmi, rmi,  r")))
> > +   (clobber (reg:CC CARRY))
> 
> Under what conditions does reload generate 2 & 3?
> We don't handle these cases for i386, so I question why
> you'd have to handle them here.

Hmmm... I took those alternatives out, and was still able to build
everything and test with no regressions.  Very strange.  Anyway, I'll
take those out.

> > +(define_insn "*bic<mode>_cg"
> > +  [(set (match_operand:QHI 0 "msp_nonimmediate_operand" "=rYs,m")
> > +	(and:QHI (match_operand:QHI 1 "msp_general_operand" "0,0")
> > +		 (match_operand 2 "msp430_inv_constgen_operator" "n,n")))]
> > +  ""
> > +  "@
> > +   BIC%x0%B0\t#%I2, %0
> > +   BIC%X0%B0\t#%I2, %0"
> > +)
> 
> This really should be an alternative of
> 
> > +(define_insn "and<mode>3"
> > +  [(set (match_operand:QHI 0 "msp_nonimmediate_operand" "=rYs,rm")
> > +	(and:QHI (match_operand:QHI 1 "msp_nonimmediate_operand" "%0,0")
> > +		 (match_operand:QHI 2 "msp_general_operand" "riYs,rmi")))
> > +   (clobber (reg:CC CARRY))
> > +   ]
> > +  ""
> > +  "@
> > +   AND%x0%B0\t%2, %0
> > +   AND%X0%B0\t%2, %0"
> > +)
> 
> ... this.

The first doesn't have a clobber, though...

> I suppose shrink-wrapping results in larger prologues and epilogues
> in general?  Otherwise I'd suggest using simple_return here.

../../gcc-trunkb/gcc/function.c: In function 'vec<edge_def*, va_heap, vl_ptr> convert_jumps_to_returns(basic_block_def*, bool, vec<edge_def*, va_heap, vl_ptr>)':
../../gcc-trunkb/gcc/function.c:5765: error: 'emit_use_return_register_into_block' was not declared in this scope
../../gcc-trunkb/gcc/function.c:5767: error: 'emit_return_into_block' was not declared in this scope
../../gcc-trunkb/gcc/function.c:5797: error: 'emit_use_return_register_into_block' was not declared in this scope
../../gcc-trunkb/gcc/function.c: In function 'basic_block_def* emit_return_for_exit(edge_def*, bool)':
../../gcc-trunkb/gcc/function.c:5839: error: 'emit_return_into_block' was not declared in this scope
../../gcc-trunkb/gcc/function.c: In function 'void thread_prologue_and_epilogue_insns()':
../../gcc-trunkb/gcc/function.c:6594: error: 'emit_return_into_block' was not declared in this scope
../../gcc-trunkb/gcc/function.c:6625: error: 'emit_return_into_block' was not declared in this scope

> Do you find these "reversed" patterns actually used?  In the deep
> dark past one needed to provide them, but I thought we'd fixed the
> middle-end not to generate them anymore...

It's not that kind of reverse.  The MSP430 doesn't have a full
complement of comparisons; sometimes we need to use a reversed-operand
comparison to make up for a missing one:

	; TRUE for comparisons we need to reverse.
	(define_predicate "msp430_reversible_cmp_operator"
	  (match_code "gt,gtu,le,leu"))

> > +; TRUE for constants which are bit positions for zero_extract
> > +(define_predicate "msp430_bitpos"
> > +  (and (match_code "const_int")
> > +       (match_test ("   INTVAL (op) >= 0
> > +		     && INTVAL (op) <= 15 "))))
> 
> IN_RANGE?

Ok.


> That's because the insn is mis-defined.  Even with unspec_volatile, you must
> respect the form of rtl.  Above, operand 1 is positioned as an input, which is
> wrong.  It must be positioned as an output, e.g.
> 
>   [(set (match_operand 0 "register_operand" "=r")
>         (unspec_volatile [(match_operand 1 "immediate_operand" "i")]
>                          UNS_POPM))]
>   "POPM%B0\t%1, %0

operand 1 indicates *one* of the registers that is popped, but not all
of them.  POPM pops a variable number of registers.  How do you
specifiy that in RTL?

> > +;; These are memory references that are safe to use with the X suffix,
> > +;; because we know/assume they need not index across the 64k boundary.
> > +(define_constraint "Ys"
> > +  "Memory reference, stack only."
> > +  (and (match_code "mem")
> > +       (ior
> > +	(and (match_code "plus" "0")
> > +	     (and (match_code "reg" "00")
> > +		  (match_test ("CONST_INT_P (XEXP (XEXP (op, 0), 1))"))
> > +		  (match_test ("IN_RANGE (INTVAL (XEXP (XEXP (op, 0), 1)), -1 << 15, (1 << 15)-1)"))))
> > +	(match_code "reg" "0")
> > +	)))
> 
> "stack-only" isn't a good description here, surely, since it supports any base
> register with minimal offset.

It's not the stack register, it's the *stack*.  We know the stack is
in the first 64k, but other objects may not be.  The MSP430X has a
feature in silicon that, if a register 'happens' to be less than 64k,
will truncate a (reg+int16) address to 64k, but if the register is
already above 64k, no truncation occurs.  I.e. the 64k wrap depends on
the *value* in the register, not the addressing mode or opcode
encoding.

So, for example, if R6 has 0xffe0 and you use [R6+0x0100], you will
get [0x00e0] not [0x100e0], but if R6 has 0x1ffe0 you get [0x200e0].

We can avoid this if we think a wrap is possible by using an int20
displacement instead, which is the other variant.

> Also, maybe better to use define_memory_constraint, since that way
> reload is told that it can reduce any address to a bare base
> register in order to satisfy the constraint?

Maybe.  Not sure if it's a win though.  For example, if you're
referencing a struct, you don't want to be doing pointer math over and
over again when a simple [reg+ofs20] will do.

> > +  /* Which registers need to be saved in the pro/epilogue.  */
> > +  int need_to_save [FIRST_PSEUDO_REGISTER];
> 
> HARD_REG_SET, surely.

Does it really matter, or is this just convention?  An array is faster
for a small fixed number of registers.

> > +#undef  TARGET_FRAME_POINTER_REQUIRED
> > +#define TARGET_FRAME_POINTER_REQUIRED msp430_frame_pointer_required
> > +
> > +static bool
> > +msp430_frame_pointer_required (void)
> > +{
> > +  return false;
> > +}
> 
> hook_bool_void_false.

Will do.

> > +bool
> > +msp430_promote_prototypes (const_tree fntype ATTRIBUTE_UNUSED)
> > +{
> > +  return false;
> > +}
> 
> hook_bool_const_tree_false

Will do.

> > +static enum machine_mode
> > +msp430_get_raw_arg_mode (int regno)
> > +{
> > +  return (regno == ARG_POINTER_REGNUM) ? VOIDmode : Pmode;
> > +}
> 
> How can the raw mode of AP be VOID?

Nick, this one's yours.