Hi all! So, should we go on with the proposed fix inside `store_bit_field`? As updating `gen_lowpart_common` seems kind of tricky, we should at least prevent `store_bit_field` from generating non-canonical subregs.
Thanks, Konstantinos On Thu, Jun 12, 2025 at 10:47 AM Konstantinos Eleftheriou <konstantinos.elefther...@vrull.eu> wrote: > > 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 > >
