[PATCH v2] bitops.h: add sign_extend8(), 16 and 64 functions

[PATCH v2] bitops.h: add sign_extend8(), 16 and 64 functions

[Martin Kepplinger]

This adds helper functions for sign-extending signed values of any lower
(hardware-)given size to s8, s16 or s64 respectively, just like sign_extend32()
for s32.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
 include/linux/bitops.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 5d858e0..9c31680 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -161,6 +161,28 @@ static inline __u8 ror8(__u8 word, unsigned int shift)
 }
 
 /**
+ * sign_extend8 - sign extend a 8-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<8) to sign bit
+ */
+static inline __s8 sign_extend8(__u8 value, int index)
+{
+	__u8 shift = 7 - index;
+	return (__s8)(value << shift) >> shift;
+}
+
+/**
+ * sign_extend16 - sign extend a 16-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<16) to sign bit
+ */
+static inline __s16 sign_extend16(__u16 value, int index)
+{
+	__u8 shift = 15 - index;
+	return (__s16)(value << shift) >> shift;
+}
+
+/**
  * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
  * @value: value to sign extend
  * @index: 0 based bit index (0<=index<32) to sign bit
@@ -171,6 +193,17 @@ static inline __s32 sign_extend32(__u32 value, int index)
 	return (__s32)(value << shift) >> shift;
 }
 
+/**
+ * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<64) to sign bit
+ */
+static inline __s64 sign_extend64(__u64 value, int index)
+{
+	__u8 shift = 63 - index;
+	return (__s64)(value << shift) >> shift;
+}
+
 static inline unsigned fls_long(unsigned long l)
 {
 	if (sizeof(l) == 4)
-- 
2.1.4

[RFC] input: gtco: use bitops' sign_extend8

[Martin Kepplinger]

Example change, using new sign_extend functions.
---
 drivers/input/tablet/gtco.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index 8580456..25b3834 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -59,7 +59,7 @@ Scott Hill shill@gtcocalcomp.com
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
 #include <asm/byteorder.h>
-
+#include <linux/bitops.h>
 
 #include <linux/usb/input.h>
 
@@ -666,13 +666,8 @@ static void gtco_urb_callback(struct urb *urbinfo)
 		case 4:
 			/* Tilt */
 
-			/* Sign extend these 7 bit numbers.  */
-			if (device->buffer[6] & 0x40)
-				device->buffer[6] |= 0x80;
-
-			if (device->buffer[7] & 0x40)
-				device->buffer[7] |= 0x80;
-
+			device->buffer[6] = sign_extend8(device->buffer[6], 6);
+			device->buffer[7] = sign_extend8(device->buffer[6], 6);
 
 			valsigned = (device->buffer[6]);
 			input_report_abs(inputdev, ABS_TILT_X, (s32)valsigned);
-- 
2.1.4

[RFC] rtc: use sign_extend8 instead of manual conversion

[Martin Kepplinger]

Example change, using new sign_extend functions.
---
 drivers/rtc/rtc-x1205.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index b1de58e..3ec0b95 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -22,6 +22,7 @@
 #include <linux/rtc.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/bitops.h>
 
 #define DRV_VERSION "1.0.8"
 
@@ -366,8 +367,7 @@ static int x1205_get_atrim(struct i2c_client *client, int *trim)
 	 * perform sign extension. The formula is
 	 * Catr = (atr * 0.25pF) + 11.00pF.
 	 */
-	if (atr & 0x20)
-		atr |= 0xC0;
+	atr = sign_extend8(atr, 5);
 
 	dev_dbg(&client->dev, "%s: raw atr=%x (%d)\n", __func__, atr, atr);
 
-- 
2.1.4

[RFC] media: stb0899: use sign_extend8 instead of manual work

[Martin Kepplinger]

Example change, using new sign_extend functions.
---
 drivers/media/dvb-frontends/stb0899_algo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/stb0899_algo.c b/drivers/media/dvb-frontends/stb0899_algo.c
index 93596e0..7bbcfde 100644
--- a/drivers/media/dvb-frontends/stb0899_algo.c
+++ b/drivers/media/dvb-frontends/stb0899_algo.c
@@ -19,6 +19,7 @@
 	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
+#include <linux/bitops.h>
 #include "stb0899_drv.h"
 #include "stb0899_priv.h"
 #include "stb0899_reg.h"
@@ -1490,9 +1491,7 @@ enum stb0899_status stb0899_dvbs2_algo(struct stb0899_state *state)
 		/* Store signal parameters	*/
 		offsetfreq = STB0899_READ_S2REG(STB0899_S2DEMOD, CRL_FREQ);
 
-		/* sign extend 30 bit value before using it in calculations */
-		if (offsetfreq & (1 << 29))
-			offsetfreq |= -1 << 30;
+		offsetfreq = sign_extend32(offset_freq, 29);
 
 		offsetfreq = offsetfreq / ((1 << 30) / 1000);
 		offsetfreq *= (internal->master_clk / 1000000);
-- 
2.1.4

[RFC] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

Example change, using new sign_extend functions.
---
 drivers/hwmon/jc42.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 388f8bc..d0582a3 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -31,6 +31,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/bitops.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = {
@@ -213,11 +214,7 @@ static u16 jc42_temp_to_reg(int temp, bool extended)
 
 static int jc42_temp_from_reg(s16 reg)
 {
-	reg &= 0x1fff;
-
-	/* sign extend register */
-	if (reg & 0x1000)
-		reg |= 0xf000;
+	reg = sign_extend16(reg, 12);
 
 	/* convert from 0.0625 to 0.001 resolution */
 	return reg * 125 / 2;
-- 
2.1.4

Re: [PATCH v2] bitops.h: add sign_extend8(), 16 and 64 functions

[Guenter Roeck]

On Mon, Jan 12, 2015 at 06:22:50PM +0100, Martin Kepplinger wrote:
> This adds helper functions for sign-extending signed values of any lower
> (hardware-)given size to s8, s16 or s64 respectively, just like sign_extend32()
> for s32.
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

Re: [RFC] hwmon: jc42: use bitops' sign_extend16

[Guenter Roeck]

On Mon, Jan 12, 2015 at 06:28:20PM +0100, Martin Kepplinger wrote:
> Example change, using new sign_extend functions.

No need to drop the Signed-off: line; it only means that I can not take
the patch even if I want to. Also, if the patch itself is final, no need
to state that it is an example in the description. Instead, use an
introductory mail (RFC PATCH 0/x] to explain the background.

Otherwise looks good; feel free to add

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

to the next version.

Thanks,
Guenter

[RFC PATCH 0/4] Example changes using proposed sign_extend functions

[Martin Kepplinger]

These are 4 of probably many examples of what could be changed if the proposed
sign_extend8, 16 and 64 are added to bitops.h, now as real patches.

Again, keep in mind, only take them if "PATCH v2 add sign_exted8, 16 and 64"
is included first.

[PATCH 1/4] input: gtco: use bitops' sign_extend8

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/gtco.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
index 8580456..25b3834 100644
--- a/drivers/input/tablet/gtco.c
+++ b/drivers/input/tablet/gtco.c
@@ -59,7 +59,7 @@ Scott Hill shill@gtcocalcomp.com
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
 #include <asm/byteorder.h>
-
+#include <linux/bitops.h>
 
 #include <linux/usb/input.h>
 
@@ -666,13 +666,8 @@ static void gtco_urb_callback(struct urb *urbinfo)
 		case 4:
 			/* Tilt */
 
-			/* Sign extend these 7 bit numbers.  */
-			if (device->buffer[6] & 0x40)
-				device->buffer[6] |= 0x80;
-
-			if (device->buffer[7] & 0x40)
-				device->buffer[7] |= 0x80;
-
+			device->buffer[6] = sign_extend8(device->buffer[6], 6);
+			device->buffer[7] = sign_extend8(device->buffer[6], 6);
 
 			valsigned = (device->buffer[6]);
 			input_report_abs(inputdev, ABS_TILT_X, (s32)valsigned);
-- 
2.1.4

[PATCH 2/4] rtc: use sign_extend8 instead of manual conversion

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/rtc/rtc-x1205.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index b1de58e..3ec0b95 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -22,6 +22,7 @@
 #include <linux/rtc.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/bitops.h>
 
 #define DRV_VERSION "1.0.8"
 
@@ -366,8 +367,7 @@ static int x1205_get_atrim(struct i2c_client *client, int *trim)
 	 * perform sign extension. The formula is
 	 * Catr = (atr * 0.25pF) + 11.00pF.
 	 */
-	if (atr & 0x20)
-		atr |= 0xC0;
+	atr = sign_extend8(atr, 5);
 
 	dev_dbg(&client->dev, "%s: raw atr=%x (%d)\n", __func__, atr, atr);
 
-- 
2.1.4

[PATCH 3/4] media: stb0899: use sign_extend8 instead of manual work

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/media/dvb-frontends/stb0899_algo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/stb0899_algo.c b/drivers/media/dvb-frontends/stb0899_algo.c
index 93596e0..7bbcfde 100644
--- a/drivers/media/dvb-frontends/stb0899_algo.c
+++ b/drivers/media/dvb-frontends/stb0899_algo.c
@@ -19,6 +19,7 @@
 	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
+#include <linux/bitops.h>
 #include "stb0899_drv.h"
 #include "stb0899_priv.h"
 #include "stb0899_reg.h"
@@ -1490,9 +1491,7 @@ enum stb0899_status stb0899_dvbs2_algo(struct stb0899_state *state)
 		/* Store signal parameters	*/
 		offsetfreq = STB0899_READ_S2REG(STB0899_S2DEMOD, CRL_FREQ);
 
-		/* sign extend 30 bit value before using it in calculations */
-		if (offsetfreq & (1 << 29))
-			offsetfreq |= -1 << 30;
+		offsetfreq = sign_extend32(offset_freq, 29);
 
 		offsetfreq = offsetfreq / ((1 << 30) / 1000);
 		offsetfreq *= (internal->master_clk / 1000000);
-- 
2.1.4

[PATCH 4/4] hwmon: jc42: use bitops' sign_extend16

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/jc42.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 388f8bc..d0582a3 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -31,6 +31,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/bitops.h>
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = {
@@ -213,11 +214,7 @@ static u16 jc42_temp_to_reg(int temp, bool extended)
 
 static int jc42_temp_from_reg(s16 reg)
 {
-	reg &= 0x1fff;
-
-	/* sign extend register */
-	if (reg & 0x1000)
-		reg |= 0xf000;
+	reg = sign_extend16(reg, 12);
 
 	/* convert from 0.0625 to 0.001 resolution */
 	return reg * 125 / 2;
-- 
2.1.4

[tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[tip-bot for Martin Kepplinger]

Commit-ID:  7e9358073d3f0ed0a028c48aa54009b3296dffc9
Gitweb:     http://git.kernel.org/tip/7e9358073d3f0ed0a028c48aa54009b3296dffc9
Author:     Martin Kepplinger <martink@posteo.de>
AuthorDate: Mon, 12 Jan 2015 18:22:50 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 18 Jan 2015 20:03:51 +0100

bitops: Add sign_extend8(), 16 and 64 functions

This adds helper functions for sign-extending signed values of any
lower (hardware-)given size to s8, s16 or s64 respectively, just like
sign_extend32() for s32.

This completes the sign_extend*() API family to work on all bit sizes
like most other bitops APIs do.

Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: John Sullivan <jsrhbz@kanargh.force9.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Maxime COQUELIN <maxime.coquelin@st.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Link: http://lkml.kernel.org/r/1421083370-24924-1-git-send-email-martink@posteo.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/bitops.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 5d858e0..9c31680 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -161,6 +161,28 @@ static inline __u8 ror8(__u8 word, unsigned int shift)
 }
 
 /**
+ * sign_extend8 - sign extend a 8-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<8) to sign bit
+ */
+static inline __s8 sign_extend8(__u8 value, int index)
+{
+	__u8 shift = 7 - index;
+	return (__s8)(value << shift) >> shift;
+}
+
+/**
+ * sign_extend16 - sign extend a 16-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<16) to sign bit
+ */
+static inline __s16 sign_extend16(__u16 value, int index)
+{
+	__u8 shift = 15 - index;
+	return (__s16)(value << shift) >> shift;
+}
+
+/**
  * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
  * @value: value to sign extend
  * @index: 0 based bit index (0<=index<32) to sign bit
@@ -171,6 +193,17 @@ static inline __s32 sign_extend32(__u32 value, int index)
 	return (__s32)(value << shift) >> shift;
 }
 
+/**
+ * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<64) to sign bit
+ */
+static inline __s64 sign_extend64(__u64 value, int index)
+{
+	__u8 shift = 63 - index;
+	return (__s64)(value << shift) >> shift;
+}
+
 static inline unsigned fls_long(unsigned long l)
 {
 	if (sizeof(l) == 4)

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[Guenter Roeck]

On 01/18/2015 11:54 AM, Linus Torvalds wrote:
> Why?
>

Probably because some of us are, unfortunately, sometimes clueless :-(.

Guenter

> The 8- and 16- bit versions are the same as the 32-bit one. This seems pointless. If you want something where the sign is in bit 3, they all return the same value, just the return type differs, but that's really a *caller* thing, no?
>
>       Linus
>
> On Jan 19, 2015 7:07 AM, "tip-bot for Martin Kepplinger" <tipbot@zytor.com <mailto:tipbot@zytor.com>> wrote:
>
>     Commit-ID:  7e9358073d3f0ed0a028c48aa54009b3296dffc9
>     Gitweb: http://git.kernel.org/tip/7e9358073d3f0ed0a028c48aa54009b3296dffc9
>     Author:     Martin Kepplinger <martink@posteo.de <mailto:martink@posteo.de>>
>     AuthorDate: Mon, 12 Jan 2015 18:22:50 +0100
>     Committer:  Ingo Molnar <mingo@kernel.org <mailto:mingo@kernel.org>>
>     CommitDate: Sun, 18 Jan 2015 20:03:51 +0100
>
>     bitops: Add sign_extend8(), 16 and 64 functions
>
>     This adds helper functions for sign-extending signed values of any
>     lower (hardware-)given size to s8, s16 or s64 respectively, just like
>     sign_extend32() for s32.
>
>     This completes the sign_extend*() API family to work on all bit sizes
>     like most other bitops APIs do.
>
>     Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com <mailto:christoph.muellner@theobroma-systems.com>>
>     Signed-off-by: Martin Kepplinger <martink@posteo.de <mailto:martink@posteo.de>>
>     Reviewed-by: Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>>
>     Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org <mailto:peterz@infradead.org>>
>     Cc: John Sullivan <jsrhbz@kanargh.force9.co.uk <mailto:jsrhbz@kanargh.force9.co.uk>>
>     Cc: Linus Torvalds <torvalds@linux-foundation.org <mailto:torvalds@linux-foundation.org>>
>     Cc: Maxime COQUELIN <maxime.coquelin@st.com <mailto:maxime.coquelin@st.com>>
>     Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com <mailto:paulmck@linux.vnet.ibm.com>>
>     Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk <mailto:linux@rasmusvillemoes.dk>>
>     Cc: Theodore Ts'o <tytso@mit.edu <mailto:tytso@mit.edu>>
>     Cc: Andrew Morton <akpm@linux-foundation.org <mailto:akpm@linux-foundation.org>>
>     Link: http://lkml.kernel.org/r/1421083370-24924-1-git-send-email-martink@posteo.de
>     Signed-off-by <http://lkml.kernel.org/r/1421083370-24924-1-git-send-email-martink@posteo.de Signed-off-by>: Ingo Molnar <mingo@kernel.org <mailto:mingo@kernel.org>>
>     ---
>       include/linux/bitops.h | 33 +++++++++++++++++++++++++++++++++
>       1 file changed, 33 insertions(+)
>
>     diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>     index 5d858e0..9c31680 100644
>     --- a/include/linux/bitops.h
>     +++ b/include/linux/bitops.h
>     @@ -161,6 +161,28 @@ static inline __u8 ror8(__u8 word, unsigned int shift)
>       }
>
>       /**
>     + * sign_extend8 - sign extend a 8-bit value using specified bit as sign-bit
>     + * @value: value to sign extend
>     + * @index: 0 based bit index (0<=index<8) to sign bit
>     + */
>     +static inline __s8 sign_extend8(__u8 value, int index)
>     +{
>     +       __u8 shift = 7 - index;
>     +       return (__s8)(value << shift) >> shift;
>     +}
>     +
>     +/**
>     + * sign_extend16 - sign extend a 16-bit value using specified bit as sign-bit
>     + * @value: value to sign extend
>     + * @index: 0 based bit index (0<=index<16) to sign bit
>     + */
>     +static inline __s16 sign_extend16(__u16 value, int index)
>     +{
>     +       __u8 shift = 15 - index;
>     +       return (__s16)(value << shift) >> shift;
>     +}
>     +
>     +/**
>        * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
>        * @value: value to sign extend
>        * @index: 0 based bit index (0<=index<32) to sign bit
>     @@ -171,6 +193,17 @@ static inline __s32 sign_extend32(__u32 value, int index)
>              return (__s32)(value << shift) >> shift;
>       }
>
>     +/**
>     + * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit
>     + * @value: value to sign extend
>     + * @index: 0 based bit index (0<=index<64) to sign bit
>     + */
>     +static inline __s64 sign_extend64(__u64 value, int index)
>     +{
>     +       __u8 shift = 63 - index;
>     +       return (__s64)(value << shift) >> shift;
>     +}
>     +
>       static inline unsigned fls_long(unsigned long l)
>       {
>              if (sizeof(l) == 4)
>

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[Linus Torvalds]

On Mon, Jan 19, 2015 at 7:54 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The 8- and 16- bit versions are the same as the 32-bit one.

Side note: the 64-bit one is actually different on 32-bit
architectures, not because it would generate a different value, but
because the 64-bit overhead likely kills you. It might be that the
compiler is smart enough that we'd *only* need the 64-bit version, but
for non-constant bit numbers, it's likely hard for gcc to generate
sane single-register versions, and so the 64-bit version is likely
fine.

But I don't see the 8-bit and 16-bit versions generating better code
on any sane architecture.

           Linus

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[Peter Zijlstra]

On Mon, Jan 19, 2015 at 07:54:22AM +1200, Linus Torvalds wrote:
> Why?
> 
> The 8- and 16- bit versions are the same as the 32-bit one. This seems
> pointless. If you want something where the sign is in bit 3, they all
> return the same value, just the return type differs, but that's really a
> *caller* thing, no?

Even for the 8bit ones? Since we have the *H and *L register we have
more 8 bit regs than we have 16/32 bit regs and it might just be worth
it.

Since these are inlines the whole calling convention which would clobber
the whole of eax can go away.

Now granted, this is all very tenuous at best. A more convincing
argument might be that of documentation; calling sign_extend32() on
something you know to be a byte might be less intuitive.

[PATCH v3 0/2] bitops.h: add sign_extend64() API and improve doc

[Martin Kepplinger]

Thanks for reviewing. I propose the following additions instead.

[PATCH 1/2] bitops.h: improve documentation for sign_extend32()

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 include/linux/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 5d858e0..336f22b 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -164,6 +164,8 @@ static inline __u8 ror8(__u8 word, unsigned int shift)
  * sign_extend32 - sign extend a 32-bit value using specified bit as sign-bit
  * @value: value to sign extend
  * @index: 0 based bit index (0<=index<32) to sign bit
+ *
+ * Safe to use for sign extending to 8 and 16 bit data types aswell.
  */
 static inline __s32 sign_extend32(__u32 value, int index)
 {
-- 
2.1.4

[PATCH 2/2] bitops.h: Add sign_extend64() API

[Martin Kepplinger]

Add sign_extend64() for sign extending any (hardware-)given signed value
greater than 32 bits to s64.

Suggested-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 include/linux/bitops.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 336f22b..2fd47c8 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -173,6 +173,17 @@ static inline __s32 sign_extend32(__u32 value, int index)
 	return (__s32)(value << shift) >> shift;
 }
 
+/**
+ * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit
+ * @value: value to sign extend
+ * @index: 0 based bit index (0<=index<64) to sign bit
+ */
+static inline __s64 sign_extend64(__u64 value, int index)
+{
+	__u8 shift = 63 - index;
+	return (__s64)(value << shift) >> shift;
+}
+
 static inline unsigned fls_long(unsigned long l)
 {
 	if (sizeof(l) == 4)
-- 
2.1.4

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[H. Peter Anvin]

On 01/19/2015 02:04 AM, Peter Zijlstra wrote:
> On Mon, Jan 19, 2015 at 07:54:22AM +1200, Linus Torvalds wrote:
>> Why?
>>
>> The 8- and 16- bit versions are the same as the 32-bit one. This seems
>> pointless. If you want something where the sign is in bit 3, they all
>> return the same value, just the return type differs, but that's really a
>> *caller* thing, no?
> 
> Even for the 8bit ones? Since we have the *H and *L register we have
> more 8 bit regs than we have 16/32 bit regs and it might just be worth
> it.

Fewer, actually.  gcc doesn't really use the H registers much, and
instead considers 8-bit values to occupy the whole register, but that
means only four are available in 32-bit mode.

	-hpa

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 01/19/2015 02:04 AM, Peter Zijlstra wrote:
> > On Mon, Jan 19, 2015 at 07:54:22AM +1200, Linus Torvalds wrote:
> >> Why?
> >>
> >> The 8- and 16- bit versions are the same as the 32-bit one. 
> >> This seems pointless. If you want something where the sign 
> >> is in bit 3, they all return the same value, just the return 
> >> type differs, but that's really a *caller* thing, no?
> > 
> > Even for the 8bit ones? Since we have the *H and *L register 
> > we have more 8 bit regs than we have 16/32 bit regs and it 
> > might just be worth it.
> 
> Fewer, actually.  gcc doesn't really use the H registers much, 

Is that true for other compilers as well?

> and instead considers 8-bit values to occupy the whole 
> register, but that means only four are available in 32-bit 
> mode.

So where are we with this? Should I consider:

  7e9358073d3f ("bitops: Add sign_extend8(), 16 and 64 functions")

NAK-ed due to having marginal benefits, or due to having no 
benefits whatsoever?

How about the two patch series from Martin Keppling - that does 
seem to be both beneficial and correct, agreed?

Thanks,

	Ingo

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[Guenter Roeck]

On 02/04/2015 11:17 PM, Ingo Molnar wrote:
>
> * H. Peter Anvin <hpa@zytor.com> wrote:
>
>> On 01/19/2015 02:04 AM, Peter Zijlstra wrote:
>>> On Mon, Jan 19, 2015 at 07:54:22AM +1200, Linus Torvalds wrote:
>>>> Why?
>>>>
>>>> The 8- and 16- bit versions are the same as the 32-bit one.
>>>> This seems pointless. If you want something where the sign
>>>> is in bit 3, they all return the same value, just the return
>>>> type differs, but that's really a *caller* thing, no?
>>>
>>> Even for the 8bit ones? Since we have the *H and *L register
>>> we have more 8 bit regs than we have 16/32 bit regs and it
>>> might just be worth it.
>>
>> Fewer, actually.  gcc doesn't really use the H registers much,
>
> Is that true for other compilers as well?
>
>> and instead considers 8-bit values to occupy the whole
>> register, but that means only four are available in 32-bit
>> mode.
>
> So where are we with this? Should I consider:
>
>    7e9358073d3f ("bitops: Add sign_extend8(), 16 and 64 functions")
>
> NAK-ed due to having marginal benefits, or due to having no
> benefits whatsoever?
>
> How about the two patch series from Martin Keppling - that does
> seem to be both beneficial and correct, agreed?
>
Do you mean the two patches improving the documentation of
sign_extend32 and adding sign_extend64 ? I thought those
would be valuable.

The discussion resulted in sign_extend32() being used for non-32-bit
operations, so that by itself was quite useful.

Guenter

Re: [tip:core/types] bitops: Add sign_extend8(), 16 and 64 functions

[H. Peter Anvin]

On 02/04/2015 11:17 PM, Ingo Molnar wrote:
>>>
>>> Even for the 8bit ones? Since we have the *H and *L register
>>> we have more 8 bit regs than we have 16/32 bit regs and it
>>> might just be worth it.
>>
>> Fewer, actually.  gcc doesn't really use the H registers much,
>
> Is that true for other compilers as well?
>

Probably.  It wrecks havoc on register allocation, and it doesn't buy 
that much, especially on 64 bits where all the registers can be used for 
8-bit operations.

	-hpa

Example use of sign_extend64()

[Martin Kepplinger]

>From a first look, there are quite some places in arch where sign extension
is done. I append one example for review although I didn't even build this.
Don't use it unreviewed!

I guess the doc-addition to sign_extend32() would be valuable for it to be
used more often in long term.

[RFC][PATCH] arch: sh: use sign_extend64() for sign extension

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 arch/sh/kernel/traps_64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/sh/kernel/traps_64.c b/arch/sh/kernel/traps_64.c
index 112ea11..4e04261 100644
--- a/arch/sh/kernel/traps_64.c
+++ b/arch/sh/kernel/traps_64.c
@@ -25,6 +25,7 @@
 #include <linux/sysctl.h>
 #include <linux/module.h>
 #include <linux/perf_event.h>
+#include <linux/bitops.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/alignment.h>
@@ -101,7 +102,7 @@ static int generate_and_check_address(struct pt_regs *regs,
 	if (displacement_not_indexed) {
 		__s64 displacement;
 		displacement = (opcode >> 10) & 0x3ff;
-		displacement = ((displacement << 54) >> 54); /* sign extend */
+		displacement = sign_extend64(displacement, 9);
 		addr = (__u64)((__s64)base_address + (displacement << width_shift));
 	} else {
 		__u64 offset;
-- 
2.1.4