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?
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;
+ }
+ }
+
/* 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;
+ }
}
if (!is_gimple_min_invariant (t) && !is_gimple_lvalue (t))
