Re: [PATCH, ARM] VSHL, VSHR, VLSHR immediate values support

[Tejas Belagod <tejas dot belagod at arm dot com>]

Hi,

I can't approve or reject this patch, but I have a few comments inline
below.

On Tue, 2010-06-22 at 16:50 +0400, Dmitry Plotnikov wrote:
> This is repost of our patch ( 
> http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00462.html), which also 
> includes a bugfix to previous patch.
> This patch adds support for immediate shift values for NEON in ARM 
> backend.  We added new predicates for immediate shift value or register 
> operands and used them in new patterns. The only tiny modification in 
> target-independent part is in optabs.c and it has been tested on x86_64 
> without any regressions.
> 



+int
+neon_immediate_valid_for_shift (rtx op, enum machine_mode mode,
+            rtx *modconst, int *elementwidth)
+{
+  unsigned int innersize = GET_MODE_SIZE (GET_MODE_INNER (mode));
+  unsigned int n_elts = CONST_VECTOR_NUNITS (op), i;
+  unsigned HOST_WIDE_INT last_elt = 0;
+
+  /* Split vector constant out into a byte vector.  */
+  for (i = 0; i < n_elts; i++)
+    {
+      rtx el = CONST_VECTOR_ELT (op, i);
+      unsigned HOST_WIDE_INT elpart;
+  
+      if (GET_CODE (el) == CONST_INT)
+        elpart = INTVAL (el);
+      else if (GET_CODE (el) == CONST_DOUBLE)
+        return 0;
+      else
+        gcc_unreachable ();
+
+      if (i != 0 && elpart != last_elt)
+        return 0;
+
+      last_elt = elpart;
+    }
+
+  /* shift less than element size */
+  if (last_elt > innersize * 8)
+    return 0;
+


VSHL allows (elemsize_in_bits - 1) and VSHR allows elemsize_in_bits as
immediate shifts. This condition here holds good for vshr, but not vshl.
It should be something like:
neon_immediate_valid_for_shift (rtx op, enum machine_mode mode,
            rtx *modconst, int *elementwidth, bool is_vshr)
{
  ....
  elemsize_in_bits = innersize * 8;
  return (is_vshr ?  last_elt <= elemsize_in_bits 
  : last_elt < elemsize_in_bits );
}

And correspondingly change neon_immediate_valid_for_shift () prototype,
definition and call points.
  
Or possibly have two predicates - one imm_for_neon_rshift_operand for
VSHR and imm_for_neon_lshift_operand for VSHL.


 (define_insn "vashl<mode>3"
+  [(set (match_operand:VDQIW 0 "s_register_operand" "=w,w")
+  (ashift:VDQIW (match_operand:VDQIW 1 "s_register_operand" "w,w")
+         (match_operand:VDQIW 2 "imm_shift_or_reg_neon" "w,Dn")))]
+  "TARGET_NEON"
+  {
+    switch (which_alternative)
+      {
+        case 0: return "vshl.<V_s_elem>\t%<V_reg>0, %<V_reg>1,
%<V_reg>2";
+        case 1: return neon_output_shift_immediate ("vshl", 's',
&operands[2],
+                         <MODE>mode, VALID_NEON_QREG_MODE
(<MODE>mode));
+        default: gcc_unreachable ();
+      }
+  }

In the second template for immediate, the ideal UAL syntax is 'i',
though 's' is allowed - proabaly a minor issue.

Thanks,
Tejas.