On Fri, Dec 15, 2017 at 10:08:03AM -0600, Qing Zhao wrote:
> a little confused here:
>
> in the current code:
> . the first case is: result = strcmp() != 0
> . the second case is: if (strcmp() != 0)
>
> so, the missing case you mentioned above is:
>
> result = if (strcmp() != 0)
>
> or something else?
result = (strcmp () != 0 ? 15 : 37);
or similar. Though, usually COND_EXPRs are added by the tree-if-conversion
pass, so you might need -ftree-loop-if-convert option and it probably needs
to be within some loop which will have just a single bb after the
if-conversion.
> >
> >> + /* When both arguments are known, and their strlens are unequal, we can
> >> + safely fold the call to a non-zero value for strcmp;
> >> + othewise, do nothing now. */
> >> + if (idx1 != 0 && idx2 != 0)
> >> + {
> >> + HOST_WIDE_INT const_string_leni1 = -1;
> >> + HOST_WIDE_INT const_string_leni2 = -1;
> >> + const_string_leni1 = compute_string_length (idx1);
> >> + const_string_leni2 = compute_string_length (idx2);
> >
> > Why do you initialize the vars when you immediately overwrite it?
>
> just a habit to declare a variable with initialization :-).
>
> > Just do
> > HOST_WIDE_INT const_string_leni1 = compute_string_length (idx1);
>
> I can change it like this.
> > etc.
> >
> >> + /* When one of args is constant string. */
> >> + tree var_string;
> >> + HOST_WIDE_INT const_string_leni = -1;
> >> +
> >> + if (idx1)
> >> + {
> >> + const_string_leni = compute_string_length (idx1);
> >> + var_string = arg2;
> >> + }
> >> + else if (idx2)
> >> + {
> >> + const_string_leni = compute_string_length (idx2);
> >> + var_string = arg1;
> >> + }
> >
> > Haven't you checked earlier that one of idx1 and idx2 is non-zero?
>
> Yes.
>
> it’s guaranteed that there is one and ONLY one of idx1 and idx2 is non-zero
> when getting here.
>
> > If so, then the else if (idx2) will just might confuse -Wuninitialized,
>
> Okay.
>
> > if you just use else, you don't need to initialize const_string_leni
> > either.
>
> I think that const_string_leni still need to be initialized in this case,
> because when idx2 is non-zero,
> const_string_leni is initialized to compute_string_length (idx2).
Sure. But
type uninitialized_var;
if (cond1)
uninitialized_var = foo;
else if (cond2)
uninitialized_var = bar;
use (uninitialized_var);
is a coding style which asks for -Wmaybe-uninitialized warnings, in order
not to warn, the compiler has to prove that cond1 || cond2 is always true,
which might not be always easy for the compiler.
> > This is something that looks problematic to me. get_range_strlen returns
> > some conservative upper bound on the string length, which is fine if
> > var_string points to say a TREE_STATIC variable where you know the allocated
> > size, or automatic variable. But if somebody passes you a pointer to a
> > structure and the source doesn't contain aggregate copying for it, not sure
> > if you can take for granted that all the bytes are readable after the '\0'
> > in the string. Hopefully at least for flexible array members and arrays in
> > such positions get_range_strlen will not provide the upper bound, but even
> > in other cases it doesn't feel safe to me.
>
> this is the part that took me most of the time during the implementation.
>
> I have considered the following 3 approaches to decide the size of the
> variable array:
>
> A. use “compute_builtin_object_size” in tree-object-size.h to decide
> the size of the
> object. However, even with the simplest case, it cannot provide the
> information.
compute_builtin_object_size with modes 0 or 1 computes upper bound, what you
are really looking for is lower bound, so that would be mode 2, though that
mode isn't actually used in real-world code and thus might be not fully
tested.
> B. use “get_range_strlen” in gimple-fold.h to decide the size of the
> object. however,
> it cannot provide valid info for simple array, either.
get_range_strlen returns you a range, the minval is not what you're looking
for, that is the minimum string length, so might be too short for your
purposes. And maxval is an upper bound, but you are looking for lower
bound, you need guarantees this amount of memory can be accessed, even if
there is 0 in the first byte.
> > Furthermore, in the comments you say that you do it only for small strings,
> > but in the patch I can't see any upper bound, so you could transform strlen
> > that would happen to return say just 1 or 2 with a function call that
> > possibly reads megabytes of data (memcmp may read all bytes, not just stop
> > at the first difference).
>
> do you mean for very short constant string, we should NOT change it to a.
> call to memcmp? instead we should just
> inline it with byte comparison sequence?
I mean we should never ever replace strcmp or strncmp call with library call to
memcmp, you don't know how it is implemented and it could access any bytes
in the new range, while previously strcmp/strncmp wouldn't. If you have
sufficient guarantees that the memory must be all mapped and thus read-only
accessible (see above, I don't think get_range_strlen provides that), it
might be fine to inline expand the memcmp some way, because there you have
full control over what the compiler will emit. But if we find out during
expansion we don't want to expand it inline, we should fall back to calling
strcmp or strncmp. Which means, instead of changing the call to the
memcmp_eq builtin, change it to some STRCMP or STRNCMP internal function,
ensure it is expanded inline the way you prefer and if it can't, it will
call strcmp or strncmp.
Jakub
