This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] constrain one character optimization to one character stores (PR 90989)


On Fri, Jun 28, 2019 at 2:16 AM Jeff Law <law@redhat.com> wrote:
>
> On 6/27/19 10:12 AM, Jakub Jelinek wrote:
> > On Thu, Jun 27, 2019 at 09:15:41AM -0600, Jeff Law wrote:
> >> Actually it was trivial to create with store merging.
> >>
> >> char x[20];
> >> foo()
> >> {
> >>   x[0] = 0x41;
> >>   x[1] = 0x42;
> >> }
> >>
> >>   MEM <unsigned short> [(char *)&x] = 16961;
> >>
> >> So clearly we can get this in gimple.  So we need a check of some kind,
> >> either on the LHS or the RHS to ensure that we actually have a single
> >> character store as opposed to something like the above.
> >
> > handle_char_store is only called if:
> >           tree type = TREE_TYPE (lhs);
> >           if (TREE_CODE (type) == ARRAY_TYPE)
> >             type = TREE_TYPE (type);
> >           if (TREE_CODE (type) == INTEGER_TYPE
> >               && TYPE_MODE (type) == TYPE_MODE (char_type_node)
> >               && TYPE_PRECISION (type) == TYPE_PRECISION (char_type_node))
> >             {
> >               if (! handle_char_store (gsi))
> >                 return false;
> >             }
> > so the above case will not trigger, the only way to call it for multiple
> > character stores is if you have an array.  And so far I've succeeded
> > creating that just with strcpy builtin.
> How I could have missed it so many times is a mystery to me.  I
> literally was looking at this on the phone with Martin today for the nth
> time and my brain just wouldn't parse this code correctly.
>
> We can only get into handle_char_store for an LHS that is a character or
> array of characters.  So the only case we have to worry about would be
> if we have a constant array on the RHS.  Something like this from Jakub:
>
>  MEM <char[2]> [(char *)&x] = MEM <char[2]> [(char *)"ab"];
>
> But in this case the RHS is going to be an ARRAY_TYPE.  And thus
> Martin's new code would reject it because it would only do the
> optmiization if the RHS has INTEGER_TYPE.
>
> So I think my concerns are ultimately a non-issue.
>
> FWIW if you're trying to get something like Jakub's assignment, this
> will do it:
>
>   char a[7];
>   int f ()
>   {
>     __builtin_strcpy (a, "654321");
>   }
>
> Which results in this being passed to handle_char_store:
>
>   MEM <unsigned char[7]> [(char * {ref-all})&a] = MEM <unsigned char[7]>
> [(char * {ref-all})"654321"];
>
> We can make it more problematical by first doing a strcpy into a so that
> we create an SI structure.  Something like this:
>
>  char a[7];
>   int f ()
>   {
>     strcpy (a, "123");
>     __builtin_strcpy (a, "654321");
>   }
>
> Compiled with -O2 -fno-tree-dse (the -fno-tree-dse ensures the first
> strcpy is not eliminated).
>
> That should get us into handle_char_store and into the new code from
> Martin's patch that wants to test:
>
>
>
> +      if (cmp > 0
> +         && storing_nonzero_p
> +         && TREE_CODE (TREE_TYPE (rhs)) == INTEGER_TYPE)
>
> But the RHS in this case has an ARRAY_TYPE and Martin's test would
> reject it.
>
>
> So at this point my concerns are alleviated.
>
> Jakub, Richi, do either of you have concerns?  I've kindof owned the
> review of this patch from Martin, but since y'all have chimed in, I'd
> like to give you another chance to chime in before I ACK.
>
> jeff
>
> ps.  FWIW, the gimple FE seems to really dislike constant strings in MEM
> expressions like we've been working with.  It also seems to go into an
> infinite loop if you've got an extra closing paren on the RHS..  It was
> still useful to poke at things a bit.  One could argue that if we get
> the FE to a suitable state that it could define what is and what is not
> valid gimple.  That would likely be incredibly help for this kind of stuff.

;)  error handling is a bit weak indeed.

I'm testing/committing the following to allow string literals in __MEM
which needed string address literals.  I guess for Fortran we need
to extend this to cover CONST_DECLs which should eventually
appear as _Literal (int *) &5 for example.

2019-07-01  Richard Biener  <rguenther@suse.de>

        c/
        * gimple-parser.c (c_parser_gimple_postfix_expression): Handle
        _Literal (char *) &"foo" for address literals pointing to
        STRING_CSTs.

        * gcc.dg/gimplefe-42.c: New testcase.

Attachment: gimplefe-addr-taken-string-literal
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]