Re: [PATCH 1/3] Fix PR46029: reimplement if-convert stores.

[Sebastian Pop <sebpop at gmail dot com>]

On Fri, Nov 5, 2010 at 07:06, Richard Guenther <rguenther@suse.de> wrote:
>> +/* Returns true when stmt contains a data reference. ?*/
>>
>> ?static bool
>> -ifcvt_memrefs_wont_trap (gimple stmt, VEC (data_reference_p, heap) *refs)
>> +has_unaligned_memory_refs (gimple stmt)
>
> Ick - unified diffs are hard to read (sometimes). ?The comment
> doesn't match the function name -- unaligned data reference or not?
>

Right, the comment is incomplete.

>> + ? ? ?if ((bitpos % BITS_PER_UNIT) != 0)
>
> Hmm, that's not actually unaligned but not addressable, right?
> I guess you want to re-use ivopts may_be_nonaddressable_p instead.

Correct.  One of the testcases that I added triggered this error in
expand, and I took part of this code from may_be_nonaddressable_p.  I
don't remember why I was not able to use may_be_nonaddressable_p, but
I will try.

>> + ? ? ?if (has_unaligned_memory_refs (stmt))
>> + ? ? {
>> + ? ? ? if (dump_file && (dump_flags & TDF_DETAILS))
>> + ? ? ? ? fprintf (dump_file, "uses misaligned memory...\n");
>
> But here it suggests misaligned again (why'd we care for misalignment?)
>

I will change this printf to nonaddressable memory.

>> +/* Insert at the beginning of the first basic block of the current
>> + ? function the allocation on the stack of N bytes of memory and
>> + ? return a pointer to this scratchpad memory. ?*/
>> +
>> +static tree
>> +create_scratchpad (void)
>> +{
>> + ?basic_block bb = single_succ (ENTRY_BLOCK_PTR);
>> + ?gimple_stmt_iterator gsi = gsi_after_labels (bb);
>> +
>> + ?/* void *tmp = __builtin_alloca */
>> + ?const char *name = "scratch_pad";
>> + ?tree x = build_int_cst (integer_type_node, 64);
>> + ?gimple stmt = gimple_build_call (built_in_decls[BUILT_IN_ALLOCA], 1, x);
>> + ?tree var = create_tmp_var (ptr_type_node, name);
>> + ?tree tmp = make_ssa_name (var, stmt);
>
> It would be better to use an automatic variable than using alloca
> which is expensive. ?Why was your choice that way? ?(Are we ever
> if-converting aggregate stores? ?I hope not.)
>
> Also you are unconditionally allocating 64 bytes instead of N.
>
> Note that if you want to make vectorization happy you would need
> to ensure that for
>
> ?if (x)
> ? ?a[i] = ...;
>
> the scratchpad you'll end up using will have the _same_ alignment
> as a[i] (same or larger for all offsets). ?Using a local array
> of chars should make it possible for the vectorizer to adjust
> its alignment if needed.
>

Ok, thanks for the recommendation, I will try to use a local array.

Also, for the vectorization to happen, we should teach the vectorizer
and the data dependence analysis that the reads/writes to the
scratchpad have inconsequential effects.  The vectorizer should also
be able to use extra scratchpad memory to transform the scratchpaded
dataref into a vector in the scratchpad.

>
> Overall I like the new way much more. ?Please update and repost.

Thanks for the review.  I will send an updated patch.

Sebastian