Re: [PATCH] tree level if-conversion for vectorizer

[Devang Patel <dpatel at apple dot com>]

On Aug 26, 2004, at 10:46 AM, Diego Novillo wrote:

> I tried not to not duplicate the feedback you got from Richard.
>
> > + /* In tree-ssa-loop-im.c  */
> > + /* The possibilities of statement movement.  */
> > +
> > + enum move_pos
> > +   {
> > +     MOVE_IMPOSSIBLE,		/* No movement -- side effect expression.  */
> > +     MOVE_PRESERVE_EXECUTION,	/* Must not cause the non-executed 
> > statement
> > +  				   become executed -- memory accesses, ... */
> > +     MOVE_POSSIBLE			/* Unlimited movement.  */
> > +   };
> Fix comment indentation.

ok

> > +    A short description of if-conversion:
> > +
> > +      o Decide if loop is if-convertable or not.
>                     ^
> 		    a

ok
> > +      o Walk all loop basic blocks in breadth first ordr (BFS order).
>                                                        ^^^^
> 						       order
>
> > +        o Remove conditional statements (at the end of basic block)
> > +          and propogate condition into destination basic blcoks'
> > + 	 predicate list.
> > +        o Replace modify expression with conditional modify 
> > expression
> > +          using current basic block's condition.
> > +      o Merge all basic blocks
> > +        o Replace phi nodes with conditional modify expr
> > +        o Merge all basic blocks into header
> Please add a sample transformation here.  Just to help the
> intuition of what we want to achieve.

ok

> > + /* Main entry point.
> > +    Apply if-conversion to the LOOP. Return true if successful 
> > otherwise return
> > +    false. If false is returned then loop remains unchanged.  */
> > +
> Document argument FOR_VECTORIZER.

ok

> The return value doesn't seem to be used anywhere.  Why is it
> important?

One reason is that If we decide to discard not-vectorized if-converted 
loop. I raised it in description. Second reason is based on decision 
when to invoke this. If auto-vectorizer requests if-conversion than it 
can take appropriate actions based on its result. For example, due to 
one bug (in if-conversion ;-) in apple-ppc-branch if-converted loop is 
not suitable for unknown loop bound vectorization.

> Similarly for most of the other comments.  There are missing
> descriptions for arguments.  Variables, arguments, etc should be
> capitalized inside comments.  Also check for spelling and grammar.

ok

> > +   /* CHECME: Do we need this?  */
> > +   compute_immediate_uses (TDFA_USE_OPS, NULL);
> > +   compute_immediate_uses (TDFA_USE_VOPS, NULL);
> No, we don't.

ok
> > +     case COND_EXPR:
> > +       /* Update destination blocks' predicate list and remove this
> > + 	 condition expression.  */
> > +       tree_if_convert_cond_expr (loop, t, cond, bsi);
> > +       cond = NULL_TREE; /* CHECKME */
> Why the CHECKME note?  Haven't you already propagated the
> condition to the destination blocks?

yup. it seems some left over, I missed.

> > + static void
> > + tree_if_convert_cond_expr (struct loop *loop, tree stmt, tree cond,
> > + 			   block_stmt_iterator *bsi)
> > + {
> > +   tree dst1, dst2, c, new_cond;
> > +   new_cond = NULL_TREE;
> > +
> > + #ifdef ENABLE_CHECKING
> > +   if (TREE_CODE (stmt) != COND_EXPR)
> > +     abort ();
> > + #endif
> > +
> > +   c = TREE_OPERAND (stmt, 0);
> > +   dst1 = TREE_OPERAND (stmt, 1);
> > +   dst2 = TREE_OPERAND (stmt, 2);
> > +
> s/dst1/then_clause/
> s/dst2/else_clause/

ok

> > + #if 0
> > +   /* If target does not support vector compare and select operations
> > +      then do not if-convert loop for vectorizer.  */
> > +   if (for_vectorizer &&
> > +       (!targetm.vect.support_vector_compare_p ()
> > +        || !targetm.vect.support_vector_select_p ()))
> > +     {
> > +       if (dump_file && (dump_flags & TDF_DETAILS))
> > + 	fprintf (dump_file, "target does not support vector 
> > compare/select\n");
> > +       return false;
> > +     }
> > + #endif
> > +
> No #if 0 code, please.

It will be used when next patch (to support vector conditional 
operations and auto-vectorization) enables use of FOR_VECTORIZER. But I 
can remove it now.

> > +   if (cond)
> > +     {
> > +       /* Attempt to do simple boolean expression simplification.  */
> > +
> Why don't you just call fold() here?

ok.

> > + static bool
> > + gate_tree_if_conversion (void)
> > + {
> > +   return true;
> Do we really want to always apply this transformation?

see above. This is why main routine is named test_if_conversion(). I 
should have renamed it before sending this patch and raise this issue, 
but you anyway asked this.

> > + }
> > +
> > + struct tree_opt_pass pass_if_conversion =
> > + {
> > +   "ifcvt",                          /* name */
> > +   gate_tree_if_conversion,         /* gate */
> > +   test_if_conversion,           /* execute */
> > +   NULL,                                       /* sub */
> > +   NULL,                                       /* next */
> > +   0,                                  /* static_pass_number */
> > +   0,                                   /* tv_id */
> > +   PROP_cfg | PROP_ssa,                        /* 
> > properties_required */
> Add PROP_alias.

ok

> > +   0,                                  /* properties_provided */
> > +   0,                                  /* properties_destroyed */
> > +   0,                                  /* todo_flags_start */
> > +   TODO_dump_func
> > +     | TODO_verify_ssa
> > +     | TODO_verify_stmts | TODO_verify_flow /* todo_flags_finish */
> > + };
> > +
> >   /* Loop optimizer finalization.  */
> Better move this to tree-if-conv.c.

ok

> > --- 615,620 ----
> > *************** build_ssa_operands (tree stmt, stmt_ann_
> > *** 833,839 ****
> >     switch (code)
> >       {
> >       case MODIFY_EXPR:
> > !       get_expr_operands (stmt, &TREE_OPERAND (stmt, 1), opf_none);
> >         if (TREE_CODE (TREE_OPERAND (stmt, 0)) == ARRAY_REF
> >   	  || TREE_CODE (TREE_OPERAND (stmt, 0)) == ARRAY_RANGE_REF
> >   	  || TREE_CODE (TREE_OPERAND (stmt, 0)) == COMPONENT_REF
> > --- 832,847 ----
> >     switch (code)
> >       {
> >       case MODIFY_EXPR:
> > !       if (TREE_CODE (TREE_OPERAND (stmt, 1)) == COND_EXPR)
> > !    	{
> > !    	  tree t_stmt = TREE_OPERAND (stmt, 1);
> > !   	  get_expr_operands (stmt, &COND_EXPR_COND (t_stmt), opf_none);
> > !    	  get_expr_operands (stmt, &TREE_OPERAND (t_stmt, 1), opf_none);
> > !    	  get_expr_operands (stmt, &TREE_OPERAND (t_stmt, 2), opf_none);
> > !    	}
> > !       else
> > ! 	get_expr_operands (stmt, &TREE_OPERAND (stmt, 1), opf_none);
> > !
> >         if (TREE_CODE (TREE_OPERAND (stmt, 0)) == ARRAY_REF
> >   	  || TREE_CODE (TREE_OPERAND (stmt, 0)) == ARRAY_RANGE_REF
> >   	  || TREE_CODE (TREE_OPERAND (stmt, 0)) == COMPONENT_REF
> No.  This goes in get_expr_operands.

[snip]
> No.  This must be handled by 'case COND_EXPR'.

ok. Richard is also saying the same.

I'll prepare another version of this patch based on comments I've 
received.
-
Devang