This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR 40049, prevent garbage collect errors on machines with vector/vector shift


On Fri, May 08, 2009 at 02:38:11PM +0200, Richard Guenther wrote:
> On Fri, May 8, 2009 at 2:09 PM, Michael Meissner
> <meissner@linux.vnet.ibm.com> wrote:
> > This patch fixes PR 40049, so that the correct tree/types are used when the
> > compiler is converting a vector << scalar shift into a vector << vector shift
> > for the machines that only have vector << vector shift (powerpc, spu -- the
> > x86_64 sse5 instruction set also has vector << vector shifts, but since they
> > also have vector << scalar shifts, they won't run into the situation as much).
> >
> > Ultimately, this is due to vect_get_vec_def_for_operand creating the vector
> > from the scalar, and the type is incorrect in the shift case. ?In particular,
> > shifts and rotates are unusual, in that they are binary operators, but the
> > types of the left and right sides are not identical (the right side is int).
> > Originally, I fixed it in vect_get_vec_df_for_operand, but it was suggested
> > that it was better to fix it in a more targetted approach. ?I rewrote the
> > patch originally to just convert the type of operand, but in some cases, it
> > fails the simple_use test if the operand is loop invariant, but not a
> > constant. ?So my third patch creates the constructor directly for that case.
> 
> Hm.  I do not think this is correct.  Why is vect_is_simple_use
> failing for you?  I guess this is because op1 is then something
> like (short) i_1?  With that you would end up generating invalid
> gimple.

Ok, the the minimal patch that is needed is just doing the convert if we are
doing constant shifts.  Alternatively, since there are only two platforms with
vector/vector shifts and no vector/scalar varient, we could just put a
vector/scalar expander in the backends, and eliminate the code in
tree-vect-stmts.c that does this conversion.

Note, variable V16QI/V8HI shifts are currently sub-optimal for all platforms in
that the compiler converts the vectors to V4SI, does the shifts, and then
converts it back to V16QI/V8HI, even though the powerpc has byte/16-bit
oriented shifts (x86 does not have byte oriented shifts until you get to SSE5).
I ran into this when I was working on the SSE5, but it never came up high
enough in priority to fix it.  I suspect the problem is somewhere upstream is
handling binary operators, and it converts both sides to the same widenend
type.  Shifts/rotates are special in that you should not do this widening, and
the type of the right side is int.  However, that is not the subject for this
bug.

Here, I'm concerned with the compiler failing due to the bad types causing
garbage collection problems in some cases, and generating incorrect code if the
GC bug is not triggered.

2009-05-07  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR tree-optimization/40049
	* tree-vect-stmts.c (vectorizable_operation): If the machine has
	only vector/vector shifts, convert the type of the constant to the
	appropriate type to avoid building incorrect trees, which
	eventually have problems with garbage collection.

Index: gcc/tree-vect-stmts.c
===================================================================
--- gcc/tree-vect-stmts.c	(revision 147246)
+++ gcc/tree-vect-stmts.c	(working copy)
@@ -1974,11 +1974,23 @@ vectorizable_operation (gimple stmt, gim
 	  else
 	    {
 	      optab = optab_for_tree_code (code, vectype, optab_vector);
-	      if (vect_print_dump_info (REPORT_DETAILS)
-		  && optab
+	      if (optab
 		  && (optab_handler (optab, TYPE_MODE (vectype))->insn_code
 		      != CODE_FOR_nothing))
-		fprintf (vect_dump, "vector/vector shift/rotate found.");
+		{
+		  tree inner_type = TREE_TYPE (vectype);
+		  tree op1_type = TREE_TYPE (op1);
+
+		  if (vect_print_dump_info (REPORT_DETAILS))
+		    fprintf (vect_dump, "vector/vector shift/rotate found.");
+
+		  /* Unlike the other binary operators, shifts/rotates have
+		     the rhs being int, instead of the same type as the lhs,
+		     so make sure the scalar is the right type if we are
+		     dealing with vectors of short/char.  */
+		  if (dt[1] == vect_constant_def)
+		    op1 = fold_convert (inner_type, op1);
+		}
 	    }
 	}
 

-- 
Michael Meissner, IBM
4 Technology Place Drive, MS 2203A, Westford, MA, 01886, USA
meissner@linux.vnet.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]