Re: [patch] Cleanup and improvement of if-conversion for vectorization

[Richard Guenther <rguenther at suse dot de>]

On Tue, 25 May 2010, Sebastian Pop wrote:

> On Tue, May 25, 2010 at 06:39, Richard Guenther <rguenther@suse.de> wrote:
> > Patch #6 is not ok (why's it needed?)
> >
> 
> I used this debug counter to find out which if-converted loop was
> causing a bootstrap compare error.  Note that this patch is not
> necessary, I could keep it in my branch, but there are other passes
> where a debug counter is used and that is included in trunk: see
> for example the RTL level if conversion.
> 
> > Patch #7 is not ok (the original code looks ok, though you might
> > want to use create_tmp_reg instead). ?Please explain why the
> > patch is needed.
> >
> 
> Same thing as for #2, patch #7 is not needed if #13 is not committed.
>
> > Patch #11. ?predicate_bbs needs an
> > overall comment on what you are doing
> 
> Ok, I will add a comment.
> 
> > (and using trees is ugly,
> > you basically un-CSE the predicates as well).
> 
> I did not modified the way BBs are predicated using the ->aux pointers.
> If you want to improve it, that would be a separate set of changes.
> 
> > + ?mark_sym_for_renaming (gimple_vop (cfun));
> > should not be necessary. ?Why do you need it?
> 
> That's an error from my side: this should be in #13, not here.
> I will move this away from patch #11.
> 
> > + ?/* Now, all statements are if-converted: combine all the basic
> > + ? ? blocks into one huge basic block. ?*/
> > ? combine_blocks (loop);
> > I thought now combine_blocks does the if-conversion.
> >
> 
> Yes, combine_blocks does the code generation: do you have a better
> name for this function?  Note that what was called tree_if_convert was
> the analysis part mixed with a bit of code generation (removal of
> conditions at the end of basic blocks).

I was refering to the comment.  Shouldn't it be
/* Now all statements are if-convertible.  Combine all the basic
   blocks into one huge basic block doing the if-conversion on-the-fly.  
*/

> > Patch #12. ?Why is loop->header not special-case?
> 
> loop->header is predicated with "true" as it gets executed for each
> iteration of the loop.  loop->header is not special cased in
> if_convertible_gimple_assign_stmt_p because we now test only the basic
> blocks that are predicated by something different than the "true"
> predicate.

Ok then.

> > ?In is_predicated
> > use ()s to properly vertically align the predicates.
> >
> 
> Ok, I will do this.

Thanks.

> > Patch #13. ?See my initial comment.
> 
> I am still thinking about that: my impression is that all the loop
> transforms that are touching the memory access order have
> potentially the same problem with concurrent threads accessing the
> same memory.  I will have to think a little bit more about why this
> if-conversion transform is different than the loop interchange, or
> different than array privatization that is even more intrusive.
> 
> > ?With the large number of patches
> > it is hard to follow the series at this point - I'm defering detailed
> > review of the rest until the previous changes are sorted out / committed.
> >
> 
> I will first prepare the set of patches that are approved, and test
> these separately.  I will then separately submit the remaining more
> controversial patches for further discussion.

Thanks.

Richard.