Re: [PATCH, aarch64] Fix pr69305 -- addti miscompilation

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Sun, Jan 24, 2016 at 03:19:35AM -0800, Richard Henderson wrote:
> As Jakub notes in the PR, the representation for add_compare and
> sub_compare were wrong.  And several of the add_carryin patterns
> were duplicates.
> 
> This adds a CC_Cmode for which only the Carry bit is valid.
> 
> The patch appears to generate moderately decent code.  For gcc7 we
> should look into why we'll prefer to mark an output REG_UNUSED
> instead of matching the pattern with that output removed.  This
> results in continuing to use adds (though simplifying adc) after
> we've proved that there will be no carry into the high part of an
> adds+adc pair.
> 
> Ok?
> 
> 
> r~

Hi Richard,

Some tiny nits below:

> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 71fc514..363785e 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -1755,6 +1755,44 @@
>    [(set_attr "type" "alus_sreg,alus_imm,alus_imm")]
>  )
>  
> +(define_insn "*add<mode>3_compare1_cconly"

I don't understand the naming scheme, it got me a wee bit confused with
add<mode>3_compare0 and friends, where the 0 indicates a comparison with
zero...

> +  [(set (reg:CC_C CC_REGNUM)
> +	(ne:CC_C
> +	  (plus:<DWI>
> +	    (zero_extend:<DWI>
> +	      (match_operand:GPI 0 "aarch64_reg_or_zero" "%rZ,rZ,rZ"))
> +	    (zero_extend:<DWI>
> +	      (match_operand:GPI 1 "aarch64_plus_operand" "r,I,J")))
> +	  (zero_extend:<DWI>
> +	    (plus:GPI (match_dup 0) (match_dup 1)))))]
> +  ""
> +  "@
> +  cmn\\t%<w>0, %<w>1
> +  cmn\\t%<w>0, %<w>1
> +  cmp\\t%<w>0, #%n1"
> +  [(set_attr "type" "alus_sreg,alus_imm,alus_imm")]
> +)
> +
> +(define_insn "add<mode>3_compare1"
> +  [(set (reg:CC_C CC_REGNUM)
> +	(ne:CC_C
> +	  (plus:<DWI>
> +	    (zero_extend:<DWI>
> +	      (match_operand:GPI 1 "aarch64_reg_or_zero" "%rZ,rZ,rZ"))
> +	    (zero_extend:<DWI>
> +	      (match_operand:GPI 2 "aarch64_plus_operand" "r,I,J")))
> +	  (zero_extend:<DWI>
> +	    (plus:GPI (match_dup 1) (match_dup 2)))))
> +   (set (match_operand:GPI 0 "register_operand" "=r,r,r")
> +	(plus:GPI (match_dup 1) (match_dup 2)))]
> +  ""
> +  "@
> +  adds\\t%<w>0, %<w>1, %<w>2
> +  adds\\t%<w>0, %<w>1, %<w>2
> +  subs\\t%<w>0, %<w>1, #%n2"
> +  [(set_attr "type" "alus_sreg,alus_imm,alus_imm")]
> +)
> +
>  (define_insn "*adds_shift_imm_<mode>"
>    [(set (reg:CC_NZ CC_REGNUM)
>  	(compare:CC_NZ

<snip>

> +;; Note that a single add with carry is matched by cinc,
> +;; and the adc_reg and csel types are matched into the same
> +;; pipelines by existing cores.

I can't see us remembering to update this comment on pipeline models
were it to ever become false. Maybe just drop it?

> @@ -2440,13 +2427,53 @@
>    [(set_attr "type" "alu_ext")]
>  )
>  
> -(define_insn "sub<mode>3_carryin"
> -  [(set
> -    (match_operand:GPI 0 "register_operand" "=r")
> -    (minus:GPI (minus:GPI
> -		(match_operand:GPI 1 "register_operand" "r")
> -		(ltu:GPI (reg:CC CC_REGNUM) (const_int 0)))
> -	       (match_operand:GPI 2 "register_operand" "r")))]
> +;; The hardware description is op1 + ~op2 + C.
> +;;                           = op1 + (-op2 + 1) + (1 - !C)
> +;;                           = op1 - op2 - 1 + 1 - !C
> +;;                           = op1 - op2 - !C.
> +;; We describe the later.

s/later/latter/

Otherwise, this is OK.

Thanks,
James