Re: [PATCH][RFC] Introduce BIT_FIELD_INSERT

[Richard Biener <rguenther@suse.de>]

On Tue, 7 Jan 2020, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Tue, 7 Jan 2020, Andrew Pinski wrote:
> >
> >> On Mon, Jan 6, 2020 at 11:36 PM Richard Biener <rguenther@suse.de> wrote:
> >> >
> >> > On Mon, 16 Dec 2019, Andrew Pinski wrote:
> >> >
> >> > > On Thu, Nov 15, 2018 at 12:31 AM Richard Biener <rguenther@suse.de> wrote:
> >> > > >
> >> > > > On Thu, 15 Nov 2018, Richard Biener wrote:
> >> > > >
> >> > > > > So - can you fix it please?  Also note that the VECTOR_CST case
> >> > > > > (as in BIT_FIELD_REF) seems to be inconsistent here and counts
> >> > > > > "bits" in a different way?
> >> > > >
> >> > > > And bonus points for documenting BIT_FIELD_REF and BIT_INSERT_EXPR
> >> > > > in generic.texi, together with those "details".
> >> > >
> >> > > This is the fix:
> >> > > diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> >> > > index 8e9e299..a919b63 100644
> >> > > --- a/gcc/fold-const.c
> >> > > +++ b/gcc/fold-const.c
> >> > > @@ -12301,6 +12301,8 @@ fold_ternary_loc (location_t loc, enum
> >> > > tree_code code, tree type,
> >> > >         {
> >> > >           unsigned HOST_WIDE_INT bitpos = tree_to_uhwi (op2);
> >> > >           unsigned bitsize = TYPE_PRECISION (TREE_TYPE (arg1));
> >> > > +         if (BYTES_BIG_ENDIAN)
> >> > > +           bitpos = TYPE_PRECISION (type) - bitpos - bitsize;
> >> > >           wide_int tem = (wi::to_wide (arg0)
> >> > >                           & wi::shifted_mask (bitpos, bitsize, true,
> >> > >                                               TYPE_PRECISION (type)));
> >> >
> >> > I guess you need to guard against BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN
> >> > as well.
> >> 
> >> Yes I will add that check.
> >> 
> >> >  Also the above only works reliably for mode-precision
> >> > integers?  We might want to disallow BIT_FIELD_REF/BIT_INSERT_EXPR
> >> > on non-mode-precision entities in the GIMPLE/GENERIC verifiers.
> >> 
> >> You added that check already for gimple in r268332 due to PR88739.
> >> BIT_FIELD_REF around tree-cfg.c:3083
> >> BIT_INSERT_EXPR  around tree-cfg.c:4324
> >
> > Ah, good ;)  Note neither BIT_FIELD_REF nor BIT_INSERT_EXPR are
> > documented in generic.texi and BIT_FIELD_REF is documented in tree.def
> > as operating on structs/unions (well, memory).  And for register args
> > we interpret it as storing the register to memory and interpreting
> > the bit positions in memory bit terms (with the store doing endian
> > fiddling).
> 
> Ah, was going to ask what the semantics were. :-)  That sounds good
> because it's essentially the same as for non-paradoxical subregs.
> We have routines like subreg_lsb_1 and subreg_offset_from_lsb that
> convert byte offsets to shift amounts, so maybe we should move them
> to code that's common to both gimple and rtl.  The BYTES_BIG_ENDIAN !=
> WORDS_BIG_ENDIAN calculation is quite subtle, so I don't think we should
> open-code it everwhere we need it.
> 
> What about the subbyte part of the bit value?  Does that always count
> from the lsb of the containing byte?  E.g. for the four bytes:
> 
>   0x12, 0x34, 0x56, 0x78
> 
> what does bit offset == 3, bit size == 7 mean for big-endian?
> 
> > But for vector (register only?) accesses we interpret
> > it as specifying lane numbers but at least BIT_FIELD_REF verifying
> > doesn't barf on bit/sizes not corresponding to exact vector lanes
> > (and I know we introduce non-matching ones via at least VIEW_CONVERT
> > "merging" into BIT_FIELD_REFs).
> 
> GCC's vector lane numbering is equivalent to array index numbering for
> all endiannesses, so these cases should still be ok for BYTES_BIG_ENDIAN
> == WORDS_BIG_ENDIAN, at least on byte boundaries.  Not sure about
> subbyte boundaries -- guess it depends on the answer to the question
> above.

I was thinking about, say a SImode extract at offset == 16, size == 32 of 
a V4SImode vector.  Is that to be interpreted as some weird shifted vector
lane or as a memory "bit" location after storing the vector to
memory?  The issue I see here is that once RTL expansion decides to
spill and interpret the offset/size in non-lane terms will there ever
be a mismatch between both?

> For BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN, any sequence that crosses
> a word boundary can lead to ranges that aren't contiguous in registers,
> unless the range starts and ends on a word boundary.  This would include
> some of those vector cases, but it could also include bitfield references
> to multiword integers.
> 
> Subregs that cross a word boundary must start and end on a word boundary,
> but I guess that would be too draconian for gimple.

Yeah.  

Richard.