[patch] Lno branch merge part 8 -- canonical induction variable creation
Sun Aug 22 18:38:00 GMT 2004
On Sun, 22 Aug 2004, Zdenek Dvorak wrote:
> first let me emphasize one thing that you probably missed (as some
> of your comments suggest): The ivcanon patch does not have problems
> with semantics of overflow of signed arithmetics, since all induction
> variables it creates are unsigned.
My apologies for confusing the issue.
In RTH's review of your patch
> > + type = TREE_TYPE (niter);
> > + niter = fold (build (PLUS_EXPR, type,
> > + niter, convert (type, integer_one_node)));
> I don't see anything in canonicalize_loop_induction_variables
> that ensures that this addition doesn't overflow. If TYPE is
> unsigned I guess it doesn't matter -- with the predecrement
> and EQ_EXPR we can iterate 2**32 times with a 32-bit variable.
> But other parts of the compiler are going to do the wrong thing
> if TYPE is signed.
To which you had originally replied
> Even in signed type it works exactly the same way.
Which sparked this entire goose chase. Clearly by now, you understand
that, by default, signed and unsigned overflow don't behave the same way.
Instead of this inflamatory comment, what you really should have said
The code in number_of_iterations_cond in tree-ssa-loop-niter.c that
initially determines the number of iterations, always returns an
> /* Count the number of iterations. */
> niter_type = unsigned_type_for (type);
And this type is used for creating the canonical induction variables.
Hence at the hunk queried by r~, the type is always guaranteed to be
unsigned, and hence overflow is well defined.
This should be sufficient to answer the reviewer's query and is
independent of whether or not you ever completely understand how
integer overflow works within GCC.
The one comment I will make, now that you've goaded me into reading
through your patch, is that all uses of "build" should really be
using "buildN" and all uses of "convert" should really be using
Hopefully, if you resubmit your patch with these changes, and RTH's
suggested tree_int_cst_compare and use of MINUS_EXPR with integer_one_node
instead of PLUS_EXPR with integer_minus_one_node, you should be one step
closer to getting this patch approved.
More information about the Gcc-patches