On Tue, 28 May 2013, Martin Jambor wrote:
> Hi,
>
> On Mon, May 27, 2013 at 10:02:19AM +0200, Richard Biener wrote:
> > On Fri, 24 May 2013, Martin Jambor wrote:
> >
> > > Hi,
> > >
> > > On Thu, May 23, 2013 at 11:38:10AM +0200, Richard Biener wrote:
> > > > On Thu, 23 May 2013, Eric Botcazou wrote:
> > > >
> > > > > > earlier this week I asked on IRC whether we could have non-top-level
> > > > > > BIT_FIELD_REFs and Richi said that we could. However, when I later
> > > > > > looked at SRA code, quite apparently it is not designed to handle
> > > > > > non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs. So
> > > > > > in
> > > > > > order to test whether that assumption is OK, I added the following
> > > > > > into the gimple verifier and ran bootstrap and testsuite of all
> > > > > > languages including Ada and ObjC++ on x86_64. It survived, which
> > > > > > makes me wondering whether we do not want it in trunk.
> > > > >
> > > > > This looks plausible to me, but I think that you ought to verify the
> > > > > real
> > > > > assumption instead, which is that the type of the 3 nodes is always
> > > > > scalar.
> > > > > The non-toplevelness of the nodes is merely a consequence of this
> > > > > property.
> > > >
> > > > Yeah. But please put the verification into tree-cfg.c:verify_expr
> > > > instead.
> > > >
> > >
> > > Like this? Also bootstrapped and tested on x86_64-linux.
> > >
> > > Thanks,
> > >
> > > Martin
> > >
> > >
> > > 2013-05-23 Martin Jambor <mjam...@suse.cz>
> > >
> > > * tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs
> > > and REALPART_EXPRs have scalar type.
> > >
> > > Index: src/gcc/tree-cfg.c
> > > ===================================================================
> > > --- src.orig/gcc/tree-cfg.c
> > > +++ src/gcc/tree-cfg.c
> > > @@ -2669,10 +2669,17 @@ verify_expr (tree *tp, int *walk_subtree
> > >
> > > case REALPART_EXPR:
> > > case IMAGPART_EXPR:
> > > + case BIT_FIELD_REF:
> > > + if (!is_gimple_reg_type (TREE_TYPE (t)))
> > > + {
> > > + error ("non-scalar BIT_FIELD_REF, IMAGPART_EXPR or REALPART_EXPR");
> > > + return t;
> > > + }
> > > + /* Fall-through. */
> > > case COMPONENT_REF:
> > > case ARRAY_REF:
> > > case ARRAY_RANGE_REF:
> > > - case BIT_FIELD_REF:
> > > case VIEW_CONVERT_EXPR:
> > > /* We have a nest of references. Verify that each of the operands
> > > that determine where to reference is either a constant or a variable,
> >
> > Yes, that looks good to me. Note that this still does not verify
> > that REALPART_EXPR, IMAGPART_EXPR and BIT_FIELD_REF are only
> > outermost handled-component refs. It merely verifies that if they
> > are outermost then they are not aggregate.
> >
> > Thus a followup would be to move the BIT_FIELD_REF handling in the
> > loop below to the above case sub-set and disallow BIT_FIELD_REF,
> > REALPART_EXPR and IMAGPART_EXPR inside that loop.
> >
> > Though I'm pretty sure that evetually this will fail ...
> >
> > The patch is ok, it's an improvement over the current state.
>
> I've committed it s revision 199379, thanks. As far as the
> non-top-levelness is concerned, the following (on top of the previous
> patch) also survives bootstrap and testsuite on x86_64 (all languages
> including Ada and Obj-C++). Do you think it would be acceptable as
> well?
With the following minor adjustment:
> Thanks,
>
> Martin
>
>
> 2013-05-27 Martin Jambor <mjam...@suse.cz>
>
> * tree-cfg.c (verify_expr): Verify that BIT_FIELD_REF, REALPART_EXPR
> and IMAGPART_EXPR do not occur within other handled_components.
>
> Index: src/gcc/tree-cfg.c
> ===================================================================
> --- src.orig/gcc/tree-cfg.c
> +++ src/gcc/tree-cfg.c
> @@ -2675,6 +2675,33 @@ verify_expr (tree *tp, int *walk_subtree
> return t;
> }
>
> + if (TREE_CODE (t) == BIT_FIELD_REF)
> + {
> + if (!host_integerp (TREE_OPERAND (t, 1), 1)
> + || !host_integerp (TREE_OPERAND (t, 2), 1))
> + {
> + error ("invalid position or size operand to BIT_FIELD_REF");
> + return t;
> + }
> + if (INTEGRAL_TYPE_P (TREE_TYPE (t))
> + && (TYPE_PRECISION (TREE_TYPE (t))
> + != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
> + {
> + error ("integral result type precision does not match "
> + "field size of BIT_FIELD_REF");
> + return t;
> + }
> + else if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> + && TYPE_MODE (TREE_TYPE (t)) != BLKmode
> + && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t)))
> + != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
> + {
> + error ("mode precision of non-integral result does not "
> + "match field size of BIT_FIELD_REF");
> + return t;
> + }
> + }
> +
t = TREE_OPERAND (t, 0);
here instead of ...
> /* Fall-through. */
> case COMPONENT_REF:
> case ARRAY_REF:
> @@ -2697,35 +2724,16 @@ verify_expr (tree *tp, int *walk_subtree
> if (TREE_OPERAND (t, 3))
> CHECK_OP (3, "invalid array stride");
> }
> - else if (TREE_CODE (t) == BIT_FIELD_REF)
> - {
> - if (!host_integerp (TREE_OPERAND (t, 1), 1)
> - || !host_integerp (TREE_OPERAND (t, 2), 1))
> - {
> - error ("invalid position or size operand to BIT_FIELD_REF");
> - return t;
> - }
> - if (INTEGRAL_TYPE_P (TREE_TYPE (t))
> - && (TYPE_PRECISION (TREE_TYPE (t))
> - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
> - {
> - error ("integral result type precision does not match "
> - "field size of BIT_FIELD_REF");
> - return t;
> - }
> - else if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
> - && !AGGREGATE_TYPE_P (TREE_TYPE (t))
> - && TYPE_MODE (TREE_TYPE (t)) != BLKmode
> - && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t)))
> - != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
> - {
> - error ("mode precision of non-integral result does not "
> - "match field size of BIT_FIELD_REF");
> - return t;
> - }
> - }
>
> t = TREE_OPERAND (t, 0);
> + if (TREE_CODE (t) == BIT_FIELD_REF
> + || TREE_CODE (t) == REALPART_EXPR
> + || TREE_CODE (t) == IMAGPART_EXPR)
> + {
> + error ("non-top-level BIT_FIELD_REF, IMAGPART_EXPR or "
> + "REALPART_EXPR");
> + return t;
> + }
... doing this after t = TREE_OPERAND (t, 0) (so, do it before).
Thanks,
Richard.
