On August 10, 2015 11:49:15 AM GMT+02:00, Nathan Sidwell <nat...@acm.org> wrote: >Richard, >I got the min/max VRP optimization I mentioned working, but it >uncovered a bug >in phiopts where that pass is constructing MIN and MAX exprs. (Funnily >enough, >I only found that failure due to a broken test of decimal fp). > >We start with the following bit of code: > >// X has unknown range here > if (X < 0) > X = 0; > if (X > 191) > X = 191; >// X known to be [0,191] here > >What's happening is that after mergephi3, (just before phiopt) we have >2: if X_6 < 0 goto 5 >3: if X_6 > 191 goto 5 >6: <EMPTY> >5: X_2 = PHI<0(2), 191(3), X_6(6)> > >X_2 has range info marking it to be [0,191] because of an earlier VRP >pass. This >is all good, but notice the PHI on X_2 has 3 incoming edges. > >phiopt first converts BB3, BB6 into a MIN_EXPR<X_6,191> which is >correct, but >it clones X_2 to hold that result. That cloning duplicates X_2's range > >information, which is not correct for the result of the MIN_EXPR. (this > >incorrect info causes my new optimization to elide a MAX_EXPR that is >actually >needed). > >Normally it would be ok, because the PHI on X_2 would usually only have >2 >incoming edges (from the conditional and the fall-through). > >The code in tree-ssa-phiopt.c (minmax_replacement) is: > result = duplicate_ssa_name (PHI_RESULT (phi), NULL); > new_stmt = gimple_build_assign (result, minmax, arg0, arg1); > gsi = gsi_last_bb (cond_bb); > gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT); > > replace_phi_edge_with_variable (cond_bb, e1, phi, result); > >Is the right solution to (a) use make_ssa_name instead of >duplicate_ssa_name, >(iff PHI has more than 2 edges?)? Or is it for (b) >replace_phi_edge_with_variable to zap RESULT's range info if the PHI >isn't >collapse completely? Or is it something else?
If stmts are effectively put under different controlling predicates then their defs have to have flow sensitive info pruned. I added a helper function for that recently (don't remember its name...). For the case in question using make SSA name would also work. > >More generally, when is it appropriate to use make_ssa_name and when to >use >duplicate_ssa_name? Duplicate duplicates all on-the-side info which is OK if the value of the new name is the same as that of the old one. You might need to prube the flow sensitive parts if its def is placed in a different location. Richard. >nathan
