RE: [PATCH] ALSA: opti9xx: shut up gcc-10 range warning

From: David Laight <David.Laight@ACULAB.COM>
To: 'Takashi Iwai' <tiwai@suse.de>
Cc: 'Arnd Bergmann' <arnd@arndb.de>, Jaroslav Kysela <perex@perex.cz>,
	"Takashi Iwai" <tiwai@suse.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] ALSA: opti9xx: shut up gcc-10 range warning
Date: Thu, 30 Apr 2020 09:52:09 +0000	[thread overview]
Message-ID: <94cc90ff07364743809698fae2ef7138@AcuMS.aculab.com> (raw)
In-Reply-To: <s5hk11xidob.wl-tiwai@suse.de>

From: Takashi Iwai
> Sent: 30 April 2020 09:25
> 
> On Thu, 30 Apr 2020 10:15:02 +0200,
> David Laight wrote:
> >
> > From: Arnd Bergmann
....
> > > +static inline void snd_miro_write_mask(struct snd_miro *chip,
> > > +		unsigned char reg, unsigned char value, unsigned char mask)
> > > +{
> > > +	unsigned char oldval = snd_miro_read(chip, reg);
> > >
> > > -#define snd_miro_write_mask(chip, reg, value, mask)	\
> > > -	snd_miro_write(chip, reg,			\
> > > -		(snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > > +	snd_miro_write(chip, reg, (oldval & ~mask) | (value & mask));
> > > +}
> >
> > Isn't that likely to add additional masking with 0xff at the call sites?
> > You will probably get better code if the arguments are 'unsigned int'.
> 
> I don't think such a micro optimization is needed.
> All registers, values and masks in the driver are 8bit, so keeping all
> unsigned char is rather an improvement of readability.

And every time you do any arithmetic they get extended to int.
And if you pass them to a function (as char) the compiler
has to mask the result of any arithmetic back to 8 bits.

On x86 the compiler can use the 'as if' rule and do 8 bit arithmetic.
But only if it can determine that the high bits aren't actually used.
On almost every other architecture you are likely to get a lot
of masking operations.

Just because the domain of a variable of 0..255 doesn't mean
that 'unsigned char' is an appropriate type for a variable.

For x86-64 (and probably others) 'unsigned int' is usually best for
anything that cannot be negative.
In particular it saves the sign extension instruction that has to
be inserted when an 'int' variable is used as an array index.

FWIW I think that somewhere else the ~mask had to be replaced
with (mask ^ 0xff) do avoid another spurious compiler warning.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)