Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns

[Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org>]

Hi Carrot,

On 26/03/11 15:14, Carrot Wei wrote:
> Index: arm.md
> ===================================================================
> --- arm.md	(revision 171337)
> +++ arm.md	(working copy)
> @@ -7115,7 +7115,18 @@
>     "@
>      cmp%?\\t%0, %1
>      cmn%?\\t%0, #%n1"
> -  [(set_attr "conds" "set")]
> +  [(set_attr "conds" "set")
> +   (set (attr "length")
> +     (if_then_else
> +       (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		 (eq (symbol_ref "which_alternative") (const_int 0)))
> +	    (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0))
> +		(and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0))
> +		     (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0))
> +			  (le (symbol_ref "INTVAL (operands[1])")
> +			      (const_int 255))))))
> +       (const_int 2)
> +       (const_int 4)))]
>   )

	How about adding an alternative only enabled for T2 that uses the `l' 
constraint and inventing new constraints for some of the constant values 
that are valid for 16 bit instructions since the `I' and `L' constraints 
have different meanings depending on whether TARGET_32BIT is valid or 
not ? We could then set the value of the length attribute accordingly. I 
don't think we can change the meaning of the I and L constraints very 
easily given the amount of churn that might be needed


>
>   (define_insn "*cmpsi_shiftsi"
> @@ -7286,7 +7297,14 @@
>     return \"b%d1\\t%l0\";
>     "
>     [(set_attr "conds" "use")
> -   (set_attr "type" "branch")]
> +   (set_attr "type" "branch")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
> +		     (le (minus (match_dup 0) (pc)) (const_int 256))))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )

	I don't think this and the other conditional branch instruction changes 
are correct. This could end up being the last instruction in an IT block 
and will automatically end up getting the 32 bit encoding and end up 
causing trouble with the length calculations. Remember the 16 bit 
encoding for the conditional instruction can't be used as the last 
instruction in an IT block.


> @@ -10256,7 +10288,26 @@
>
>       return \"\";
>     }"
> -  [(set_attr "type" "store4")]
> +  [(set_attr "type" "store4")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(ne (symbol_ref "{
> +		    int i, regno, hi_reg;
> +		    int num_saves = XVECLEN (operands[2], 0);
> +		    regno = REGNO (operands[1]);
> +		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> +			&&  (regno != LR_REGNUM);
> +		    for (i = 1; i<  num_saves; i++)

	
	(i < num_saves && (hi_reg == 0)) - what's the point of going through 
the register list when hi_reg != 0 in this case ? A comment to explain 
that the length calculation *must* be kept in sync with the template 
above might be useful.

cheers
Ramana