Message ID | 2672026.bC0PhPkkHn@polaris |
---|---|
State | New |
Headers | show |
Series | Fix internal error on store to FP component at -O2 | expand |
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
diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c index c8e1877f540..65e27f38fd4 100644 --- a/gcc/gimple-ssa-store-merging.c +++ b/gcc/gimple-ssa-store-merging.c @@ -1867,19 +1867,22 @@ merged_store_group::can_be_merged_into (store_immediate_info *info) if (stores[0]->rhs_code == BIT_INSERT_EXPR && info->rhs_code == INTEGER_CST) return true; - /* We can turn MEM_REF into BIT_INSERT_EXPR for bit-field stores. */ + /* We can turn MEM_REF into BIT_INSERT_EXPR for bit-field stores, but do it + only for small regions since this can generate a lot of instructions. */ if (info->rhs_code == MEM_REF && (stores[0]->rhs_code == INTEGER_CST || stores[0]->rhs_code == BIT_INSERT_EXPR) && info->bitregion_start == stores[0]->bitregion_start - && info->bitregion_end == stores[0]->bitregion_end) + && info->bitregion_end == stores[0]->bitregion_end + && info->bitregion_end - info->bitregion_start < MAX_BITSIZE_MODE_ANY_INT) return true; if (stores[0]->rhs_code == MEM_REF && (info->rhs_code == INTEGER_CST || info->rhs_code == BIT_INSERT_EXPR) && info->bitregion_start == stores[0]->bitregion_start - && info->bitregion_end == stores[0]->bitregion_end) + && info->bitregion_end == stores[0]->bitregion_end + && info->bitregion_end - info->bitregion_start < MAX_BITSIZE_MODE_ANY_INT) return true; return false; @@ -4172,6 +4175,15 @@ imm_store_chain_info::output_merged_store (merged_store_group *group) const HOST_WIDE_INT end_gap = (try_bitpos + try_size) - (info->bitpos + info->bitsize); tree tem = info->ops[0].val; + if (!INTEGRAL_TYPE_P (TREE_TYPE (tem))) + { + const unsigned HOST_WIDE_INT size + = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (tem))); + tree integer_type + = build_nonstandard_integer_type (size, UNSIGNED); + tem = gimple_build (&seq, loc, VIEW_CONVERT_EXPR, + integer_type, tem); + } if (TYPE_PRECISION (TREE_TYPE (tem)) <= info->bitsize) { tree bitfield_type @@ -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) { /* Bypass a conversion to the bit-field type. */ if (!bit_not_p