On 5/30/25 12:12 AM, Richard Biener wrote:
On Thu, May 29, 2025 at 12:27 PM Konstantinos Eleftheriou
<konstantinos.elefther...@vrull.eu> wrote:
Hi Richard, thanks for the response.
On Mon, May 26, 2025 at 11:55 AM Richard Biener <rguent...@suse.de> wrote:
On Mon, 26 May 2025, Konstantinos Eleftheriou wrote:
In `store_bit_field_1`, when the value to be written in the bitfield
and/or the bitfield itself have vector modes, non-canonical subregs
are generated, like `(subreg:V4SI (reg:V8SI x) 0)`. If one them is
a scalar, this happens only when the scalar mode is different than the
vector's inner mode.
This patch tries to prevent this, using vec_set patterns when
possible.
I know almost nothing about this code, but why does the patch
fixup things after the fact rather than avoid generating the
SUBREG in the first place?
That's what we are doing, we are trying to prevent the non-canonical
subreg generation (it's not always possible). But, there are cases
where these types of subregs are passed into `store_bit_field` by its
caller, in which case we choose not to touch them.
ISTR it also (unfortunately) depends on the target which forms
are considered canonical.
But, the way that we interpret the documentation, the
canonicalizations are machine-independent. Is that not true? Or,
specifically for the subregs that operate on vectors, is there any
target that considers them canonical?
I'm also not sure you got endianess right for all possible
values of SUBREG_BYTE. One more reason to not generate such
subreg in the first place but stick to vec_select/concat.
The only way that we would generate subregs are from the calls to
`extract_bit_field` or `store_bit_field_1` and these should handle the
endianness. Also, these subregs wouldn't operate on vectors. Do you
mean that something could go wrong with these calls?
I wanted to remark that endianess WRT memory order (which is
what store/extract_bit_field deal with) isn't always the same as
endianess in register order (which is what vec_concat and friends
operate on). If we can avoid transitioning from one to the other
this will help avoid mistakes.
In general it would be more obvious (to me) if you fixed the callers
that create those subregs.
Now, I didn't want to pretend I'm reviewing the patch - so others please
do that (as said, I'm not familiar enough with the code to tell whether
it's actually correct).
Well, I'd tend to think your core concern is correct -- if something is
generating a non-canonical SUBREG, then that needs to be fixed.
If I understand Konstantinos's comments correctly they're tackling one
of those paths and instead generating a correct form. My understanding
is also that this patch doesn't catch all the known cases.
I don't think we anyone anymore that *really* knows the code in
question. We're kind of slogging along as best as we can, but it's not
an ideal situation.
WRT fixing this earlier in the callers, I think it's the right thing to
check. So I think that means understanding how we get into this routine
with VALUE being a SUBREG and FIELDMODE being a vector mode.
Jeff
