After looking at this again, I found out that in both cases, inside `store_bit_field` and in the callers, the subregs are generated by `gen_lowpart_common` and a call to `lowpart_subreg`. So, theoretically, fixing this inside `gen_lowpart_common`, preventing the use of `lowpart_subreg` when dealing with vectors, would cover both of those cases.
The problem is that, from what I’m seeing, `gen_lowpart_common` doesn’t directly modify the RTL and returns a single RTX. For the vector cases we would need to generate multiple vector operations to access the low part of a register. Another less concerning issue, is that we won’t have access to functions like `extract_bit_field`, that we use in our implementation. So, if we can’t find a better solution, we could at least apply this one to prevent `store_bit_field` from generating more of those subregs. Konstantinos On Mon, Jun 2, 2025 at 6:44 AM Jeff Law <jeffreya...@gmail.com> wrote: > > > > 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 >
