On Thu, Jan 9, 2020 at 11:17 AM Bin.Cheng <amker.ch...@gmail.com> wrote: > > On Wed, Jan 8, 2020 at 7:50 PM Richard Biener > <richard.guent...@gmail.com> wrote: > > > > On Wed, Jan 8, 2020 at 12:30 PM Bin.Cheng <amker.ch...@gmail.com> wrote: > > > > > > On Wed, Jan 8, 2020 at 6:31 PM Richard Biener > > > <richard.guent...@gmail.com> wrote: > > > > > > > > On Wed, Jan 8, 2020 at 6:01 AM bin.cheng <bin.ch...@linux.alibaba.com> > > > > wrote: > > > > > > > > > > Sorry, here is the patch. > > > > > ------------------------------------------------------------------ > > > > > Sender:bin.cheng <bin.ch...@linux.alibaba.com> > > > > > Sent At:2020 Jan. 8 (Wed.) 12:58 > > > > > Recipient:GCC Patches <gcc-patches@gcc.gnu.org> > > > > > Subject:[PATCH GCC11]Improve uninitialized warning with value range > > > > > info > > > > > > > > > > > > > > > Hi, > > > > > > > > > > Function use_pred_not_overlap_with_undef_path_pred of > > > > > pass_late_warn_uninitialized > > > > > checks if predicate of variable use overlaps with predicate of > > > > > undefined control flow path. > > > > > For now, it only checks ssa_var comparing against constant, this can > > > > > be improved where > > > > > ssa_var compares against another ssa_var with value range info, as > > > > > described in comment: > > > > > > > > > > + /* Check value range info of rhs, do following transforms: > > > > > + flag_var < [min, max] -> flag_var < max > > > > > + flag_var > [min, max] -> flag_var > min > > > > > + > > > > > + We can also transform LE_EXPR/GE_EXPR to LT_EXPR/GT_EXPR: > > > > > + flag_var <= [min, max] -> flag_var < [min, max+1] > > > > > + flag_var >= [min, max] -> flag_var > [min-1, max] > > > > > + if no overflow/wrap. */ > > > > > > > > > > This change can avoid some false warning. Bootstrap and test on > > > > > x86_64, any comment? > > > > > > > > Definitely a good idea - the refactoring makes the patch hard to > > > > follow though. The > > > > original code seems to pick any (the "first") compare against a > > > > constant. You > > > > return the "first" but maybe from range info that might also be > > > > [-INF,+INF]. It seems > > > > that we'd want to pick the best so eventually sort the predicate chain > > > > so that compares > > > > against constants come first at least? Not sure if it really makes a > > > > difference but > > > I don't know either, but I simply tried to not break existing code int > > > the patch. > > > Function prune_uninit_phi_opnds is called for the first compares against > > > constant, actually it should be called for each comparison, but I guess > > > it's > > > just avoiding O(M*N) complexity here. > > > > Yeah. I'm just worried finding a "bad" value-range predicate cuts the > > search > > in a way that causes extra bogus warnings? > hmm, the code is now as: > + cmp_code = find_var_cmp_const (preds, phi, &flag_def, &boundary_cst, > false); > + if (cmp_code == ERROR_MARK) > + cmp_code = find_var_cmp_const (preds, phi, &flag_def, &boundary_cst, > true); > + if (cmp_code == ERROR_MARK) > return false; > > First call to find_var_cmp_const preserves the original behavior, > while the second call > only finds value range case if there is no comparison cont is found by > the first call. > So there is no extra bogus warnings?
Ah, didn't understand that part (and didn't try too hard). Possibly warrants a comment. > > > > > > > > > > even currently we could have i < 5, i < 1 so the "better" one later? > > > > It might also make > > > > sense to simply push three predicates for i < j, namely i < j (if ever > > > > useful), i < min(j) > > > > and max(i) < j to avoid repeatedly doing the range computations. > > > IIUC, with current implementation, it's not useful to check value rang > > > info for both sides of comparison because prune_uninit_phi_opnds > > > requires the flag_var be defined by PHI node in the same basic block > > > as PHI parameter. > > > > Yes, but without remembering the code very well my suggestion allows > > "new" predicates to be gathered during collecting phase while your patch > > adjusts the query phase? > I am not quite follow here. Do you mean we collect three cases "i < > j", "i < min(j)", "max(i) < j" then > call prune_uninit_phi_opnds for all three conditions? No, I've meant to somehow arrange that the 'preds' passed to use_pred_not_overlap_with_undef_path_pred contain all three predicates rather than just i < j, thus "expand" fully symbolic predicates. > This is another question? because now we simply break out of for loop > for finding such condition: > > - if ((gimple_code (flag_def) == GIMPLE_PHI) > - && (gimple_bb (flag_def) == gimple_bb (phi)) > - && find_matching_predicate_in_rest_chains (the_pred, preds, > - num_preds)) > - break; > > It's always possible that this flag_def can't prune use predicates > against undefined path predicates, while a later one can prune but is > skipped? I don't follow but I also don't want to understand the code too much ;) I'm fine with the idea and if the patch cannot introudce extra bogus warnings let's go with it. Can you amed the comment before the two find_var_cmp_const calls? I wonder whether eliding the second sweep when the first didn't find any predicate it skipped is worth the trouble. Thanks, Richard. > Thanks, > bin > > > > Richard. > > > > > Thanks, > > > bin > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > Thanks, > > > > > bin > > > > > > > > > > 2020-01-08 Bin Cheng <bin.li...@linux.alibaba.com> > > > > > > > > > > * tree-ssa-uninit.c (find_var_cmp_const): New function. > > > > > (use_pred_not_overlap_with_undef_path_pred): Call above.
