Re: Bad choices by expand_mult_highpart

[James Morrison <ja2morri at csclub dot uwaterloo dot ca>]

Roger Sayle <roger@eyesopen.com> writes:

> On 21 Mar 2004, James Morrison wrote:
> > Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de> writes:
> > > However, it would appear that the problem is caused by trying to do
> > > a signed multiply, which synth_mult doesn't actually support.  To fix
> > > this requires an adjustment like the one performed by
> > > expand_mult_highpart_adjust.
> > >
> > > Also, we need to pass WIDER_MODE instead of MODE to
> > > choose_mult_variant, so that the test whether negate_variant is
> > > allowed uses the correct mode size.
> > >
> > > This in turn made the default multiplication cost computation
> > > in choose_mult_variant result in too high values, so I simply
> > > added another argument to pass in the cost bound instead.
> > >
> > > The resulting patch is attached; it fixes my test case, but I haven't
> > > completed full testing yet.  Could you try whether it fixes your
> > > bootstrap problem on sparc?
> > >
> > > Richard, do you think this is a correct solution?  Does is get
> > > the cost calculations right for your targets?
> >
> > Thanks, my bootstrap completed with this patch.
> 
> That's good enough for me.  Ulrich can you good ahead and commit your
> patch to mainline?  Many thanks for fixing this so quickly.
> 
> 
> > > 	* expmed.c (choose_mult_variant): Pass MULT_COST as argument instead
> > > 	of using register multiplication cost.
> > > 	(expand_mult): Adapt choose_mult_variant call.
> > > 	(expand_mult_highpart): Call choose_mult_variant with WIDER_MODE
> > > 	instead of MODE; pass appropriate cost bound.  Adjust result when
> > > 	performing signed multiplication by a negative constant.
> 
> Approved.
> 
> Ideally, it would be nice to add some execution testcases to the GCC
> testsuite to see if we can catch this failure in future.  Is there a
> real problem with testsuite coverage or is the erroneous code just
> ever triggered on the platforms Richard's patch was test on?
> 
> Roger
> --

 I think this is a platform issue.  I grepped around testsuite/gcc.dg for
div, Div, quo, and ' / ', and didn't find anything that tested this, but it is
tested in real.c .

The test case for this problem is:

/* { dg-do run } */
/* Divide a signed number by something that isn't a power of two */
int main (int argc, char *argv[])
{
  volatile int i = 1;
  return i / 10; 
}

Jim