On Mon, Apr 30, 2018 at 7:16 PM, Jeff Law <l...@redhat.com> wrote:
> On 01/09/2018 02:41 PM, Martin Sebor wrote:
>> I found a few problems in the previous revision:
>>
>> 1) It didn't handle the simple case of member arrays of const
>> struct objects (only member arrays of const arrays of structs
>> were handled).
>> 2) The array_ctor_elt() function returned a narrow empty string
>> for an uninitialized CONSTRUCTOR element of any character type
>> when it should return the same string in the expected character
>> type (char, wchar_t, etc.)
>> 3) The string_constant() function would in some cases use a byte
>> offset to get the initializer from a CONSTRUCTOR instead of
>> an array index.
>>
>> The attached version 3 of the patch corrects these issues.
>> Retested on x86_64 and with the Glibc ToT.
>>
>>> After sleeping on it I realized that although enhancing
>>> gimple_fold_builtin_strlen is an improvement, it only benefits
>>> straight calls to strlen and nothing else. Calls to strcmp,
>>> sprintf, or strcpy (and along with it the rest of the strlen
>>> pass) are still expanded as if the argument were unknown. To
>>> improve even those callers, the folding needs to be done at
>>> a lower level (otherwise they'd all have to duplicate the same
>>> code as gimple_fold_builtin_strlen). With that in mind I've
>>> moved the logic to string_constant() so all of those clients
>>> benefit.
>>>
>>> Retested on x86_64-linux. Out of paranoia I also built and
>>> tested the top of Glibc trunk with no unusual failures.
>>>
>>> Martin
>>
>>
>> gcc-83693.diff
>>
>>
>> PR tree-optimization/83693 - missing strlen optimization for array of arrays
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/83693
>> * expr.c (array_ctor_elt): New function.
>> (string_constant): Call it. Handle initializers of arrays of arrays.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/83693
>> * gcc.dg/strcmp-2.c: New test.
>> * gcc.dg/strlenopt-42.c: New test.
>> * gcc.dg/strlenopt-43.c: New test.
>>
>> diff --git a/gcc/expr.c b/gcc/expr.c
>> index cd1e57d..75110e5 100644
>> --- a/gcc/expr.c
>> +++ b/gcc/expr.c
>> @@ -62,7 +62,7 @@ along with GCC; see the file COPYING3. If not see
>> #include "rtl-chkp.h"
>> #include "ccmp.h"
>> #include "rtx-vector-builder.h"
>> -
>> +#include "gimple-fold.h"
>>
>> /* If this is nonzero, we do not bother generating VOLATILE
>> around volatile memory references, and we are willing to
>> @@ -11343,6 +11343,50 @@ is_aligning_offset (const_tree offset, const_tree
>> exp)
>> return TREE_CODE (offset) == ADDR_EXPR && TREE_OPERAND (offset, 0) == exp;
>> }
>>
>> +/* Return initializer element IDX for the array CONSTRUCTOR initializer
>> + INIT or an empty string constant with type CHARTYPE if no such element
>> + exists. If IDX is null, simply return an empty string. If IDX is not
>> + constant, return NULL_TREE. A helper of string_constant. */
>> +
>> +static tree
>> +array_ctor_elt (tree chartype, tree init, tree idx)
>> +{
>> + if (idx)
>> + {
>> + if (!tree_fits_uhwi_p (idx))
>> + return NULL_TREE;
>> +
>> + HOST_WIDE_INT i = tree_to_uhwi (idx);
>> +
>> + if (i < CONSTRUCTOR_NELTS (init)
>> + && tree_int_cst_equal (CONSTRUCTOR_ELT (init, i)->index, idx))
>> + return CONSTRUCTOR_ELT (init, i)->value;
>> +
>> + tree index, value;
>> + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init), i, index, value)
>> + {
>> + if (tree_int_cst_equal (index, idx))
>> + return value;
>> + }
> So you first look at IDX and if it patches you return the appropriate value.
>
> Else you iterate from the start of the constructor list through the
> elements until you find a match.
>
> Would a binary search between 0..IDX be better here? Do we have any
> imposed ordering on the elements?
There is also get_array_ctor_element_at_index you can simply use.
>> + }
>> +
>> + /* Build and return a STRING_CST representing the empty string with
>> + CHARTYPE. Make sure the string representation has enough zero
>> + bytes for CHARTYPE. */
>> + const char nuls[16] = "";
>> + unsigned elemsize = tree_to_uhwi (TYPE_SIZE_UNIT(chartype));
>> + tree str = build_string (elemsize, nuls);
>> + tree elemtype = build_qualified_type (chartype, TYPE_QUAL_CONST);
>> + tree indextype = build_index_type (size_zero_node);
>> + tree arraytype = build_array_type (elemtype, indextype);
>> + TREE_TYPE (str) = arraytype;
>> + TREE_CONSTANT (str) = true;
>> + TREE_READONLY (str) = true;
>> + TREE_STATIC (str) = true;
> I'm a but surprised we don't have a suitable string constant lying
> around, but I don't see one.
It's ugly. Is it really necessary to do this?
Richard.
>
> So really the only concern is the compile-time cost of array_ctor_elt.
> If we know anything about the ordering of elements within the
> constructor, then we could do a lot better WRT the compile-time cost.
>
> jeff
