Re: [RFC] Teach vectorizer to deal with bitfield reads

["Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>]

Hi Richard,

Thanks for the review, I don't completely understand all of the below, 
so I added some extra questions to help me understand :)

On 27/07/2022 12:37, Richard Biener wrote:
> On Tue, 26 Jul 2022, Andre Vieira (lists) wrote:
>
> I don't think this is a good approach for what you gain and how
> necessarily limited it will be.  Similar to the recent experiment with
> handling _Complex loads/stores this is much better tackled by lowering
> things earlier (it will be lowered at RTL expansion time).
I assume the approach you are referring to here is the lowering of the 
BIT_FIELD_DECL to BIT_FIELD_REF in the vect_recog part of the 
vectorizer. I am all for lowering earlier, the reason I did it there was 
as a 'temporary' approach until we have that earlier loading.
>
> One place to do this experimentation would be to piggy-back on the
> if-conversion pass so the lowering would happen only on the
> vectorized code path.
This was one of my initial thoughts, though the if-conversion changes 
are a bit more intrusive for a temporary approach and not that much 
earlier. It does however have the added benefit of not having to make 
any changes to the vectorizer itself later if we do do the earlier 
lowering, assuming the lowering results in the same.

The 'only on the vectorized code path' remains the same though as 
vect_recog also only happens on the vectorized code path right?
>   Note that the desired lowering would look like
> the following for reads:
>
>    _1 = a.b;
>
> to
>
>    _2 = a.<representative for b>;
>    _1 = BIT_FIELD_REF <2, ...>; // extract bits
I don't yet have a well formed idea of what '<representative for b>' is 
supposed to look like in terms of tree expressions. I understand what 
it's supposed to be representing, the 'larger than bit-field'-load. But 
is it going to be a COMPONENT_REF with a fake 'FIELD_DECL' with the 
larger size? Like I said on IRC, the description of BIT_FIELD_REF makes 
it sound like this isn't how we are supposed to use it, are we intending 
to make a change to that here?

> and for writes:
>
>    a.b = _1;
>
> to
>
>    _2 = a.<representative for b>;
>    _3 = BIT_INSERT_EXPR <_2, _1, ...>; // insert bits
>    a.<representative for b> = _3;
I was going to avoid writes for now because they are somewhat more 
complicated, but maybe it's not that bad, I'll add them too.
> so you trade now handled loads/stores with not handled
> BIT_FIELD_REF / BIT_INSERT_EXPR which you would then need to
> pattern match to shifts and logical ops in the vectorizer.
Yeah that vect_recog pattern already exists in my RFC patch, though I 
can probably simplify it by moving the bit-field-ref stuff to ifcvt.
>
> There's a separate thing of actually promoting all uses, for
> example
>
> struct { long long x : 33; } a;
>
>   a.a = a.a + 1;
>
> will get you 33bit precision adds (for bit fields less than 32bits
> they get promoted to int but not for larger bit fields).  RTL
> expansion again will rewrite this into larger ops plus masking.
Not sure I understand why this is relevant here? The current way I am 
doing this would likely lower a  bit-field like that to a 64-bit load  
followed by the masking away of the top 31 bits, same would happen with 
a ifcvt-lowering approach.
>
> So I think the time is better spent in working on the lowering of
> bitfield accesses, if sufficiently separated it could be used
> from if-conversion by working on loop SEME regions.
I will start to look at modifying ifcvt to add the lowering there. Will 
likely require two pass though because we can no longer look at the 
number of BBs to determine whether ifcvt is even needed, so we will 
first need to look for bit-field-decls, then version the loops and then 
look for them again for transformation, but I guess that's fine?
> The patches
> doing previous implementations are probably not too useful anymore
> (I find one from 2011 and one from 2016, both pre-dating BIT_INSERT_EXPR)
>
> Richard.