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.
