Do not allow non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs

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  <mjambor@suse.cz>

	* tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs
	and REALPART_EXPRs have scalar type.

> 2013-05-23  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs
> 	and REALPART_EXPRs have scalar type.

I cannot formally approve, but this looks the right test to me.

On Fri, May 24, 2013 at 05:05:52PM +0200, Eric Botcazou wrote:
> > 2013-05-23  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs
> > 	and REALPART_EXPRs have scalar type.
> 
> I cannot formally approve, but this looks the right test to me.

I agree it is desirable, but is it enough to ensure that they will be only
toplevel?  Can't you e.g. do a VIEW_CONVERT_EXPR from an integer type or
floating/vector type etc. to struct type, then the verifier wouldn't
discover there is VIEW_CONVERT_EXPR <struct S, BIT_FIELD_REF<whatever, 0, 32>>?

	Jakub

> I agree it is desirable, but is it enough to ensure that they will be only
> toplevel?  Can't you e.g. do a VIEW_CONVERT_EXPR from an integer type or
> floating/vector type etc. to struct type, then the verifier wouldn't
> discover there is VIEW_CONVERT_EXPR <struct S, BIT_FIELD_REF<whatever, 0,
> 32>>?

Sure, you can apply VIEW_CONVERT_EXPR to whatever you want, but does that 
count as non-toplevelness?  Won't SRA already punt on such abomination?

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  <mjambor@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.

Thanks,
Richard.