On Tue, Jul 23, 2024 at 08:42:24AM +0200, Richard Biener wrote:
> On Tue, 23 Jul 2024, Jakub Jelinek wrote:
> > The folding into REALPART_EXPR is correct, used only when the mem_offset
> > is zero, but for IMAGPART_EXPR it didn't check the exact offset value (just
> > that it is not 0).
> > The following patch fixes that by using IMAGPART_EXPR only if the offset
> > is right and using BITFIELD_REF or whatever else otherwise.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk, 14.2
> > and other release branches?
>
> I think this is not enough - what kind of GIMPLE does this result in?
If it is
__builtin_memmove (&g, 2 + (char *) &c, 2);
then
_1 = &c + 2;
_3 = MEM <unsigned short> [(char * {ref-all})_1];
is optimized to
_3 = IMAGPART_EXPR <c_6(D)>;
as before. If it is
__builtin_memmove (&g, 1 + (char *) &c, 2);
from the testcase, then
_1 = &c + 1;
_3 = MEM <unsigned short> [(char * {ref-all})_1];
is optimized to
_3 = BIT_FIELD_REF <c_6(D), 16, 8>;
and that is expanded correctly.
> You should amend the checking in non_rewritable_mem_ref_base as well,
> it should fail when the corresponding rewrite doesn't work.
That is already the case I believe.
non_rewritable_mem_ref_base rejects it in the VECTOR_TYPE/COMPLEX_TYPE
case (so doesn't return NULL), but then falls through the
/* For integral typed extracts we can use a BIT_FIELD_REF. */
check and returns NULL_TREE there.
But then maybe_rewrite_mem_ref_base triggers already on the COMPLEX_TYPE
case.
> I suspect it falls through to the BIT_FIELD_REF code?
>
> That said, can you match up the offset check with that of
> non_rewritable_mem_ref_base then, particularly
>
> if ((VECTOR_TYPE_P (TREE_TYPE (decl))
> || TREE_CODE (TREE_TYPE (decl)) == COMPLEX_TYPE)
> && useless_type_conversion_p (TREE_TYPE (base),
> TREE_TYPE (TREE_TYPE (decl)))
> && known_ge (mem_ref_offset (base), 0)
> && known_gt (wi::to_poly_offset (TYPE_SIZE_UNIT (TREE_TYPE
> (decl))),
> mem_ref_offset (base))
> && multiple_p (mem_ref_offset (base),
> wi::to_poly_offset (TYPE_SIZE_UNIT (TREE_TYPE
> (base)))))
>
> I suppose this check should be adjusted to use the three arg multiple_p
> and check the factor against 0/1 for COMPLEX_TYPE.
Isn't that just too complex/expensive for something as simple as
mem_ref_offset (base) is 0 or TYPE_SIZE_UNIT?
That
integer_zerop () || tree_int_cst_equal looked much cheaper.
Sure, it could be done on poly_int_cst instead if that looks better:
&& (known_eq (mem_ref_offset (base), 0)
|| known_eq (mem_ref_offset (base),
wi::to_poly_offset (TYPE_SIZE_UNIT (TREE_TYPE (decl))))
And shouldn't we cache those mem_ref_offset (base) and
wi::to_poly_offset (TYPE_SIZE_UNIT (TREE_TYPE (decl)))
which are used in multiple spots?
Anyway, yet another option because non_rewritable_mem_ref_base has
the VECTOR/COMPLEX types cases together would be to have them together
in maybe_rewrite_mem_ref_base too, so do:
if ((VECTOR_TYPE_P (TREE_TYPE (sym))
|| TREE_CODE (TREE_TYPE (sym)) == COMPLEX_TYPE)
&& useless_type_conversion_p (TREE_TYPE (*tp),
TREE_TYPE (TREE_TYPE (sym)))
&& multiple_p (mem_ref_offset (*tp),
wi::to_poly_offset (TYPE_SIZE_UNIT (TREE_TYPE (*tp)))))
{
if (VECTOR_TYPE_P (TREE_TYPE (sym)))
*tp = build3 (BIT_FIELD_REF, TREE_TYPE (*tp), sym,
TYPE_SIZE (TREE_TYPE (*tp)),
int_const_binop (MULT_EXPR,
bitsize_int (BITS_PER_UNIT),
TREE_OPERAND (*tp, 1)));
else
*tp = build1 (integer_zerop (TREE_OPERAND (*tp, 1))
? REALPART_EXPR : IMAGPART_EXPR,
TREE_TYPE (*tp), sym);
}
Jakub
