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.
