Re: [PATCH] pass constants as first argument to st_mult()

From: Jeff King <peff@peff.net>
Date: Aug 1, 2016, 16:47 UTC
Message-ID: <20160801164723.mober7em6znt56w4@sigill.intra.peff.net>
In-Reply-To: <579CEF77.9070202@web.de>

On Sat, Jul 30, 2016 at 08:18:31PM +0200, René Scharfe wrote:

> The result of st_mult() is the same no matter the order of its
> arguments.  It invokes the macro unsigned_mult_overflows(), which
> divides the second parameter by the first one.  Pass constants
> first to allow that division to be done already at compile time.

I'm not opposed to this, as it's easy to do (though I suspect new calls may be introduced that violate it).

But if we really are worried about the performance of st_mult(), I think:

  static inline size_t st_mult(size_t a, size_t b)
  {
	size_t result;
	if (!__builtin_mul_overflow(a, b, &result))
		die("whoops!");
	return result;
  }

is the right direction. I just haven't gotten around to producing a polished patch.

Show 10 quoted lines

> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 58ac0a5..73d003a 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -541,7 +541,7 @@ void diffcore_rename(struct diff_options *options)
>  				rename_dst_nr * rename_src_nr, 50, 1);
>  	}
>  
> -	mx = xcalloc(st_mult(num_create, NUM_CANDIDATE_PER_DST), sizeof(*mx));
> +	mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));

I didn't look at all of the calls, but I wonder if it is a natural pattern to put the constant second. Since multiplication is commutative, it would be correct for st_mult() to just flip the order of arguments it feeds to unsigned_mult_overflows().

That may introduce the same inefficiency in other callsites, but I wonder if it would be fewer.

-Peff