On 8/9/19 5:42 PM, Martin Sebor wrote: >>> @@ -3408,7 +3457,13 @@ static bool >>> } >>> gimple *stmt = SSA_NAME_DEF_STMT (exp); >>> - if (gimple_code (stmt) != GIMPLE_PHI) >>> + if (gimple_assign_single_p (stmt)) >>> + { >>> + tree rhs = gimple_assign_rhs1 (stmt); >>> + return count_nonzero_bytes (rhs, offset, nbytes, lenrange, >>> nulterm, >>> + allnul, allnonnul, snlim); >>> + } >>> + else if (gimple_code (stmt) != GIMPLE_PHI) >>> return false; >> What cases are you handling here? Are there any cases where a single >> operand expression on the RHS affects the result. For example, if we've >> got a NOP_EXPR which zero extends RHS? Does that change the nonzero >> bytes in a way that is significant? >> >> I'm not opposed to handling single operand objects here, I'm just >> concerned that we're being very lenient in just stripping away the >> operator and looking at the underlying object. > > I remember adding the code because of a test failure but not > the specifics anymore. No tests fail with it removed so it > may not be needed. As you know, I've been juggling a few > enhancements in this area and copying code between them as > I need it so it's possible that I copied too much, or that > some other change has obviated it, or also that the test > failed somewhere else and I forgot to copy the test along > with the code I'll remove it until it's needed. Let's pull it for now. If we come across the need again, we can obviously revisit with a testcase.
> >>> @@ -3795,7 +3824,14 @@ handle_store (gimple_stmt_iterator *gsi) >>> } >>> else >>> si->nonzero_chars = build_int_cst (size_type_node, offset); >>> - si->full_string_p = full_string_p; >>> + >>> + /* Set FULL_STRING_P only if the length of the strings being >>> + written is the same, and clear it if the strings have >>> + different lengths. In the latter case the length stored >>> + in si->NONZERO_CHARS becomes the lower bound. >>> + FIXME: Handle the upper bound of the length if possible. */ >>> + si->full_string_p = full_string_p && lenrange[0] == lenrange[1]; >> So there seems to be a disconnect between the comment and the code. >> >> The comment indicates you care about the lengths of the two strings >> being the same. But what you're really comparing when the lenrange[0] >> == lenrange[1] test is that the min and max of RHS are the same. > > The comment tries to make clear that all the arrays on the RHS > of the assignment must have the same length in order to set > FULL_STRING_P. Like here where LENRANGE = { 4, 4, 4 }: > > void f (char *s) > { > if (__builtin_strlen (s) != 2) > return; > > *(int*)a = i ? 0x11111111 : 0x22222222; > } > > but not here where LENRANGE = { 1, 4, 4 }: > > *(int*)a = i < 0 ? 0x11111111 : i ? 0x22220022 : 0x33003333; > > If the bounds of the range of lengths of all the strings on > the RHS are the same they're all the same length. > > I'm open to phrasing it better. Oh, I think I see what I was missing. In the case where RHS is a conditional (or perhaps a SSA_NAME which was set from a PHI) LENRANGE will have the min/max/# bytes for the RHS was a whole, not just a single component of the RHS. >> It generally looks reasonable, so I think we just need to reach a >> conclusion on the gimple_assign_single_p cases we're trying to handle >> and the possible mismatch between the comment and the code. > > Do you want me to post another revision with > the gimple_assign_single_p test removed? I think remove that hunk, bootstrap, test, commit and post for archival purposes. I do not think another round of review is necessary. jeff