Fix internal error on store to FP component at -O2

Hi,

the attached Ada testcase triggers a GIMPLE verification failure at -O2 or 
above because the GIMPLE store merging pass generates a NOP_EXPR between a FP 
type and an integral type.  This happens when the bit-field insertion path is 
taken for a FP field, which can happen in Ada for bit-packed record types.

It is fixed by generating an intermediate VIEW_CONVERT_EXPR.  The patch also 
tames a little the bit-field insertion path because, for bit-packed record 
types in Ada, you can end up with large bit-field regions, which results in a 
lot of mask-and-shifts instructions.

Tested on x86-64/Linux, OK for the mainline?


2020-05-25  Eric Botcazou  <ebotcazou@adacore.com>

	* gimple-ssa-store-merging.c (merged_store_group::can_be_merged_into):
	Only turn MEM_REFs into bit-field stores for small bit-field regions.
	(imm_store_chain_info::output_merged_store): Be prepared for sources
	with non-integral type in the bit-field insertion case.
	(pass_store_merging::process_store): Use MAX_BITSIZE_MODE_ANY_INT as
	the largest size for the bit-field case.


2020-05-25  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt84.adb: New test.

On Mon, May 25, 2020 at 12:33 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> Hi,
>
> the attached Ada testcase triggers a GIMPLE verification failure at -O2 or
> above because the GIMPLE store merging pass generates a NOP_EXPR between a FP
> type and an integral type.  This happens when the bit-field insertion path is
> taken for a FP field, which can happen in Ada for bit-packed record types.
>
> It is fixed by generating an intermediate VIEW_CONVERT_EXPR.  The patch also
> tames a little the bit-field insertion path because, for bit-packed record
> types in Ada, you can end up with large bit-field regions, which results in a
> lot of mask-and-shifts instructions.
>
> Tested on x86-64/Linux, OK for the mainline?

Hmm, MAX_BITSIZE_MODE_ANY_INT looks a bit arbitrary since on
x86 it (IIRC) includes things like OImode. Maybe MOVE_MAX or
MAX_FIXED_MODE_SIZE are better suited here?

Of course the size of the whole region isn't what matters but how
many (if more than one) "region piece" (each word_mode size?)
a store crosses?  That is, do we want to prevent store-merging
across such boundaries?  Ah, that's the

@@ -4788,7 +4800,7 @@ pass_store_merging::process_store (gimple *stmt)
          && bitsize.is_constant (&const_bitsize)
          && ((const_bitsize % BITS_PER_UNIT) != 0
              || !multiple_p (bitpos, BITS_PER_UNIT))
-         && const_bitsize <= 64)
+         && const_bitsize <= MAX_BITSIZE_MODE_ANY_INT)
        {

hunk to which I have similar concerns.

The IL correctness fix looks OK to me but it smells like we
might have issues with inserting into x86 XFmode fields
where ordinary XFmode stores store less bytes than
an integer mode of the same size?  That issue should be
latent of course (unless it always ICEd before).  Also
it's likely reproducible only on Ada(?).

Thanks,
Richard.

>
> 2020-05-25  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimple-ssa-store-merging.c (merged_store_group::can_be_merged_into):
>         Only turn MEM_REFs into bit-field stores for small bit-field regions.
>         (imm_store_chain_info::output_merged_store): Be prepared for sources
>         with non-integral type in the bit-field insertion case.
>         (pass_store_merging::process_store): Use MAX_BITSIZE_MODE_ANY_INT as
>         the largest size for the bit-field case.
>
>
> 2020-05-25  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt84.adb: New test.
>
> --
> Eric Botcazou

> Hmm, MAX_BITSIZE_MODE_ANY_INT looks a bit arbitrary since on
> x86 it (IIRC) includes things like OImode. Maybe MOVE_MAX or
> MAX_FIXED_MODE_SIZE are better suited here?

You're right, MAX_FIXED_MODE_SIZE is better, I'm going to change it.

> The IL correctness fix looks OK to me but it smells like we
> might have issues with inserting into x86 XFmode fields
> where ordinary XFmode stores store less bytes than
> an integer mode of the same size?  That issue should be
> latent of course (unless it always ICEd before).  Also
> it's likely reproducible only on Ada(?).

Yes, this can only happen in Ada because it effectively can have bit-fields of 
any type, and not only of integral types.

> Hmm, MAX_BITSIZE_MODE_ANY_INT looks a bit arbitrary since on
> x86 it (IIRC) includes things like OImode. Maybe MOVE_MAX or
> MAX_FIXED_MODE_SIZE are better suited here?

I forgot to mention that I picked MAX_BITSIZE_MODE_ANY_INT because of:

  bool invalid = (base_addr == NULL_TREE
		  || (maybe_gt (bitsize,
				(unsigned int) MAX_BITSIZE_MODE_ANY_INT)
		      && TREE_CODE (rhs) != INTEGER_CST
		      && (TREE_CODE (rhs) != CONSTRUCTOR
			  || CONSTRUCTOR_NELTS (rhs) != 0)));

On Mon, May 25, 2020 at 2:14 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Hmm, MAX_BITSIZE_MODE_ANY_INT looks a bit arbitrary since on
> > x86 it (IIRC) includes things like OImode. Maybe MOVE_MAX or
> > MAX_FIXED_MODE_SIZE are better suited here?
>
> I forgot to mention that I picked MAX_BITSIZE_MODE_ANY_INT because of:
>
>   bool invalid = (base_addr == NULL_TREE
>                   || (maybe_gt (bitsize,
>                                 (unsigned int) MAX_BITSIZE_MODE_ANY_INT)
>                       && TREE_CODE (rhs) != INTEGER_CST
>                       && (TREE_CODE (rhs) != CONSTRUCTOR
>                           || CONSTRUCTOR_NELTS (rhs) != 0)));

That's likely because of the encoding limits of wide_ints I guess?  That is,
an implementation limit rather than one for code-quality.

Richard.

> --
> Eric Botcazou