On Fri, 2 Sep 2011, Eric Botcazou wrote:
> > But they _do_ overflow as my debugging showed, caused by that exact
> > same extract_muldiv_1 function here:
> >
> > case PLUS_EXPR: case MINUS_EXPR:
> > /* See if we can eliminate the operation on both sides. If we can,
> > we
> > can return a new PLUS or MINUS. If we can't, the only remaining
> > cases where we can do anything are if the second operand is a
> > constant. */
> > ...
> > /* If this was a subtraction, negate OP1 and set it to be an
> > addition.
> > This simplifies the logic below. */
> > if (tcode == MINUS_EXPR)
> > {
> > tcode = PLUS_EXPR, op1 = negate_expr (op1);
> >
> > the unconditional negation of op1 for tcode == MINUS_EXPR overflows
> > all sizetype values (well, all unsigned values).
>
> Only if sizetypes are no longer sign-extended, though; otherwise, I don't see
> the problem (and we certainly never detected one). Does something really go
> wrong with the unpatched compiler?
Well, even when sign-extended there is a constant you can't negate
without overflow. I would start digging for a testcase with
such case - but as said, testcases involving TYPE_IS_SIZETYPE are
very hard to generate for me.
> > Please please add some _testcases_ then that would fail when I test
> > this kind of patches.
>
> Performance testcases are hard to extract beforehand. It's only when you
> have
> a regression that you really start to dig.
Well, but I'd expect you can have a set of Ada types, a function that
returns just its size and some scan-tree-dumps that check those sizes
are folded to a constant. Or to just N statements.
I at least did verify that the case producing the error still folds
to a constant size, even after my patch.
> > The patch fixed a real bug (it's just that it is hard (for me) to
> > produce testcases that involve sizetype computations - it always
> > requires Ada to expose those bugs). So, I beg to differ.
>
> Well, on the one hand you ask for testcases in difficult cases, and on the
> other hand you don't provide one when you already have something at hand.
>
> The elimination of the sign-extension for sizetypes is probably a progress
> overall, although this will very likely cause a few problems in Ada. But
> removing working code (albeit admittedly hard to grasp) without testcases or
> real experiments is a different thing.
It definitely makes it easier to later point to this patch causing issues
compared to lumping all the "fixes" into the final patch, which, btw.
I just sent out.
If you insist I can revert the patch and apply it together with the
sign-extension change.
Richard.
