This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Thank you very much for the review and explanations. > > + /* Stop conditions: > > + 1. Constant. */ > > + if (host_integerp (expr, 1)) > > You want TREE_CONSTANT, surely. Fixed. > > > + tree access_fn = analyze_scalar_evolution (loop, expr); > > + if (access_fn == chrec_dont_know /* No access_fn. */ > > + > > + /* Not enough information: may be not loop invariant. > > + E.g., for a[b[i]], we get a[D], where D=b[i]. EXPR is D, its > > + initial_condition is D, but it depends on i - loop's induction > > + variable. */ > > + || (((init = initial_condition_in_loop_num (access_fn, loop->num)) > > + == expr) > > + && !IS_EMPTY_STMT (def_stmt = SSA_NAME_DEF_STMT (init)) > > + && flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt))) > > + > > + /* Evolution is not constant. */ > > + || ((evolution = evolution_part_in_loop_num (access_fn, loop->num)) > > + && !host_integerp (evolution, 0))) > > + { > > + *step = NULL_TREE; > > + *misalign = NULL_TREE; > > + *initial_offset = NULL_TREE; > > + return false; > > + } > > Please break up this conditional. Like so. > > *step = NULL_TREE; > *misalign = NULL_TREE; > *initial_offset = NULL_TREE; > > Move these to the beginning of the function (at least before the > SSA_NAME conditional, and then the various failure paths out of > the function only need to "return false". > > if (access_fn == chrec_dont_know) > return false; > > init = initial_condition_in_loop_num (access_fn, loop->num); > if (init != expr) > { > def_stmt = SSA_NAME_DEF_STMT (init); > if (!IS_EMPTY_STMT (def_stmt) > && flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt))) > return false; > } > > evolution = evolution_part_in_loop_num (access_fn, loop->num); > if (evolution && !host_integerp (evolution, 0)) > return false; > > Resist the temptation to embed assignments inside conditionals. > Three is really just over-the-top too much. > Done. > Do you actually want TREE_CONSTANT there at the end too? Not > having looked at what the interface of evolution_part_in_loop_num > is, I'm not sure. I changed it to if (evolution && TREE_CODE (evolution) != INTEGER_CST) since I saw that evolution can be also REAL_CST. > > For the rest, let's just take as written that I'd like you to > re-examine all other ocurrences of host_integerp and defend them. > I removed all the occurrences of host_integerp, I replaced them with TREE_CONSTANT or with TREE_CODE check as above. I removed checks for overflow, since we just combine different offsets/steps/misalignments, that I don't think can overflow. > > + /* Assumption: in MULT_EXPR the left operand is a variable > and the right > > + operand is a constant. Therefore, LEFT_OFFSET can be either a variable > > + or a constant, and RIGHT_OFFSET is always a constant. */ > > On what do you base this assumption? Arrays of variable sized types > will have no constant operand. Has something above this point already > discarded this as a possibility? > I check that RIGHT_OFFSET is constant for now. > > > r~ Thanks, Ira Patch 2 (new): (See attached file: patch2New) Diff between old patch 2 and the new one: (See attached file: patch2.diff)
Attachment:
patch2New
Description: Binary data
Attachment:
patch2.diff
Description: Binary data
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |