Re: PATCH RFC: Fix ARM bug by splitting up iwmmxt_movsi_insn

[Richard Earnshaw <rearnsha at arm dot com>]

> Richard Earnshaw <rearnsha@arm.com> writes:
> 
> > I think perhaps the best way to handle this is to force IF conversion to 
> > fail by defining a macro that will force the conditionalization to fail 
> > (for example, to make IFCVT_MODIFY_INSN return a NULL pattern for the 
> > problem cases).  Another alternative is to provide the predicated version 
> > of the move insn explicitly, and then to ensure that that version rejects 
> > the impossible cases (the predicable attribute simply means that the code 
> > generator will generate the predicated version of the pattern directly 
> > from the non-predicated version).
> 
> Your alternative suggestion makes a lot of sense to me.  This patch
> passes the C torture compile tests, and appears to do the right thing
> with some simple test cases I constructed.  The register constraints
> should tell reload how to handle the instruction.  Any thoughts?

This is ok, with a couple of minor changes.

First of all, this statement isn't quite accurate:

> The register constraints
> should tell reload how to handle the instruction.  Any thoughts?

In fact, reload will never see the cond-exec pattern, since it isn't 
generated until after reload is completed (at least, not in the current 
incarnation of the compiler).  Instead, the register constraints tell the 
if-convert pass whether the alternative can be converted into a 
conditionally executed instruction.   What is happening is this: we have 
the rtl

	(set (pc) (if_then_else (cond)
				(label)
				(pc)))

	(set (mem:SI dst) (reg:SI src))

        (code_label (lable)

(or some slightly longer version of that)

The compiler tries to convert this into

	(cond_exec (~cond) (set (mem:SI dest) (reg:SI src)))

but after doing a tentative conversion it must match the generated 
pattern.  Your modified version will match provided that there is 
constraint that matches the allocated registers.  By leaving out the 
problem alternative from the conditional execution variant then the 
pattern will no-longer match and conversion of the sequence to conditional 
execution will be abandoned.


> Index: iwmmxt.md
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/config/arm/iwmmxt.md,v
> retrieving revision 1.2
> diff -u -r1.2 iwmmxt.md
> --- iwmmxt.md	28 Jun 2003 19:43:00 -0000	1.2
> +++ iwmmxt.md	13 Oct 2003 07:56:23 -0000
> @@ -124,6 +124,42 @@
>     (set_attr "conds" "clob")]
>  )
>  
> +;; Because iwmmxt_movsi_insn is not predicable, we produce the
> +;; cond_exec version directly, with appropriate constraints.  This
> +;; will force any predicated store of a iWMMXt general register to be
> +;; done as an unconditional copy to an ARM general register followed
> +;; by a predicated store to memory.
> +
> +(define_insn "*cond_iwmmxt_movsi_insn"
> +  [(cond_exec
> +     (match_operator 2 "arm_comparison_operator"
> +      [(match_operand 3 "cc_register" "")
> +      (const_int 0)])
> +     (set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r, m,z,r,?z")
> +	  (match_operand:SI 1 "general_operand"      "rI,K,mi,r,r,z,m")))]
> +  "TARGET_REALLY_IWMMXT
> +   && (   register_operand (operands[0], SImode)
> +       || register_operand (operands[1], SImode))"
> +  "*
> +   switch (which_alternative)
> +   {
> +   case 0: return \"mov%?\\t%0, %1\";
> +   case 1: return \"mvn%?\\t%0, #%B1\";
> +   case 2: return \"ldr%?\\t%0, %1\";
> +   case 3: return \"str%?\\t%1, %0\";
> +   case 4: return \"tmcr%?\\t%0, %1\";
> +   case 5: return \"tmrc%?\\t%0, %1\";
> +   default: return arm_output_load_gr (operands);
> +  }"
> +  [(set_attr "type"           "*,*,load,store1,*,*,load")
> +   (set_attr "length"         "*,*,*,        *,*,*,  16")
> +   (set_attr "pool_range"     "*,*,4096,     *,*,*,1024")
> +   (set_attr "neg_pool_range" "*,*,4084,     *,*,*,   *")
                                                      ^^^^
This should probably be 1012 (in the base pattern, this appears to be 
mis-placed on to the following alternative).

> +   ;; We don't want to predicate this instruction, as it is already
> +   ;; predicated.
> +   (set_attr "predicable"     "no")]

All arm md patterns default to not predicable, so this is unnecessary.
> +)
> +
>  (define_insn "movv8qi_internal"
>    [(set (match_operand:V8QI 0 "nonimmediate_operand" "=y,m,y,?r,?y,?r")
>  	(match_operand:V8QI 1 "general_operand"       "y,y,m,y,r,i"))]

You also need to update the comment against the "predicable" attribute in 
the base pattern to explain that predication is done explicitly because 
some alternatives are excluded.

If you can fix the constraints on this (and the base pattern), then this 
can go in.

R.