Hi,

Whilst running a bootstrap with extra options to force bitfield 
vectorization '-O2 -ftree-vectorize -ftree-loop-if-convert 
-fno-vect-cost-model' I ran into an ICE in vect-patterns where a 
bit_field_ref had a container that wasn't INTEGRAL_TYPE and had a 
E_BLKmode, which meant we failed to build an integer type with the same 
size. For that reason I added a check to bail out earlier if the 
TYPE_MODE of the container is indeed E_BLKmode. The pattern for the 
bitfield inserts required no change as we currently don't support 
containers that aren't integer typed.

Also changed a testcase because in BIG-ENDIAN it was not vectorizing due 
to a different size of container that wasn't supported.

This passes the same bootstrap and regressions on aarch64-none-linux and 
no regressions on aarch64_be-none-elf either.

I assume you are OK with these changes Richard, but I don't like to 
commit on Friday in case something breaks over the weekend, so I'll 
leave it until Monday.

Thanks,
Andre

On 29/09/2022 08:54, Richard Biener wrote:
> On Wed, Sep 28, 2022 at 7:32 PM Andre Vieira (lists) via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> Made the change and also created the ChangeLogs.
> OK if bootstrap / testing succeeds.
>
> Thanks,
> Richard.
>
>> gcc/ChangeLog:
>>
>>           * tree-if-conv.cc (if_convertible_loop_p_1): Move ordering of
>> loop bb's from here...
>>           (tree_if_conversion): ... to here.  Also call bitfield lowering
>> when appropriate.
>>           (version_loop_for_if_conversion): Adapt to enable loop
>> versioning when we only need
>>           to lower bitfields.
>>           (ifcvt_split_critical_edges): Relax condition of expected loop
>> form as this is checked earlier.
>>           (get_bitfield_rep): New function.
>>           (lower_bitfield): Likewise.
>>           (bitfields_to_lower_p): Likewise.
>>           (need_to_lower_bitfields): New global boolean.
>>           (need_to_ifcvt): Likewise.
>>           * tree-vect-data-refs.cc (vect_find_stmt_data_reference):
>> Improve diagnostic message.
>>           * tree-vect-patterns.cc (vect_recog_temp_ssa_var): Add default
>> value for last parameter.
>>           (vect_recog_bitfield_ref_pattern): New.
>>           (vect_recog_bit_insert_pattern): New.
>>
>> gcc/testsuite/ChangeLog:
>>
>>           * gcc.dg/vect/vect-bitfield-read-1.c: New test.
>>           * gcc.dg/vect/vect-bitfield-read-2.c: New test.
>>           * gcc.dg/vect/vect-bitfield-read-3.c: New test.
>>           * gcc.dg/vect/vect-bitfield-read-4.c: New test.
>>           * gcc.dg/vect/vect-bitfield-read-5.c: New test.
>>           * gcc.dg/vect/vect-bitfield-read-6.c: New test.
>>           * gcc.dg/vect/vect-bitfield-write-1.c: New test.
>>           * gcc.dg/vect/vect-bitfield-write-2.c: New test.
>>           * gcc.dg/vect/vect-bitfield-write-3.c: New test.
>>           * gcc.dg/vect/vect-bitfield-write-4.c: New test.
>>           * gcc.dg/vect/vect-bitfield-write-5.c: New test.
>>
>> On 28/09/2022 10:43, Andre Vieira (lists) via Gcc-patches wrote:
>>> On 27/09/2022 13:34, Richard Biener wrote:
>>>> On Mon, 26 Sep 2022, Andre Vieira (lists) wrote:
>>>>
>>>>> On 08/09/2022 12:51, Richard Biener wrote:
>>>>>> I'm curious, why the push to redundant_ssa_names?  That could use
>>>>>> a comment ...
>>>>> So I purposefully left a #if 0 #else #endif in there so you can see
>>>>> the two
>>>>> options. But the reason I used redundant_ssa_names is because ifcvt
>>>>> seems to
>>>>> use that as a container for all pairs of (old, new) ssa names to
>>>>> replace
>>>>> later. So I just piggy backed on that. I don't know if there's a
>>>>> specific
>>>>> reason they do the replacement at the end? Maybe some ordering
>>>>> issue? Either
>>>>> way both adding it to redundant_ssa_names or doing the replacement
>>>>> inline work
>>>>> for the bitfield lowering (or work in my testing at least).
>>>> Possibly because we (in the past?) inserted/copied stuff based on
>>>> predicates generated at analysis time after we decide to elide something
>>>> so we need to watch for later appearing uses.  But who knows ... my mind
>>>> fails me here.
>>>>
>>>> If it works to replace uses immediately please do so.  But now
>>>> I wonder why we need this - the value shouldn't change so you
>>>> should get away with re-using the existing SSA name for the final value?
>>> Yeah... good point. A quick change and minor testing seems to agree.
>>> I'm sure I had a good reason to do it initially ;)
>>>
>>> I'll run a full-regression on this change to make sure I didn't miss
>>> anything.
>>>