| Message ID | 20241204013620.862943-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: afe: rescale: A few cleanups | expand |
From: Andy Shevchenko > Sent: 04 December 2024 01:33 > > There are two (non-critical) issues with the code. First of all, > the eXclusive OR is not defined for booleans, so boolean to integer > promotion is required, Second, the u32 variable is used to keep > boolean value, so boolean is converted implicitly to the integer. Except there is no such thing as 'boolean' they are all integers. And the compiler has to have some set of rules to handle the cases where the memory that hold the 'boolean' doesn't have the value 0 or 1. > > All these are not needed when variable is defined as boolean to begin > with and operations are replaced by simple != and ||. > > Fixes: 701ee14da95d ("iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/iio/afe/iio-rescale.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index b6a46036d5ea..470dd7d41b2a 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -26,7 +26,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > int _val, _val2; > s32 rem, rem2; > u32 mult; > - u32 neg; > + bool neg; > > switch (scale_type) { > case IIO_VAL_INT: > @@ -95,7 +95,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, > * If only one of the rescaler elements or the schan scale is > * negative, the combined scale is negative. > */ > - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { > + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) { That is wrong, the || would also need to be !=. Which will all generate real pile of horrid code. (I think the x86 version will stun you.) I'm guessing that somewhere there is a: neg = value < 0; Provided all the values are the same size (eg int/s32), in which case: neg = value; ... if ((neg ^ rescale->numerator ^ rescale->denominator) < 0) will be the desired test. David > if (*val) > *val = -*val; > else > -- > 2.43.0.rc1.1336.g36b5255a03ac > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Dec 06, 2024 at 01:24:09PM +0000, David Laight wrote: > From: Andy Shevchenko > > Sent: 04 December 2024 01:33 > > > > There are two (non-critical) issues with the code. First of all, > > the eXclusive OR is not defined for booleans, so boolean to integer > > promotion is required, Second, the u32 variable is used to keep > > boolean value, so boolean is converted implicitly to the integer. > > Except there is no such thing as 'boolean' they are all integers. I believe this is an exercise in linguistics as I'm not native speaker but I am very well aware of the promotions to the integer values. > And the compiler has to have some set of rules to handle the cases > where the memory that hold the 'boolean' doesn't have the value 0 or 1. No doubts. ... > > * If only one of the rescaler elements or the schan scale is > > * negative, the combined scale is negative. > > */ > > - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { > > + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) { > > That is wrong, the || would also need to be !=. Why do you think so? Maybe it's comment(s) that is(are) wrong? > Which will all generate real pile of horrid code. > (I think the x86 version will stun you.) I think your remark is based on something, can you show the output to elaborate what exactly becomes horrible in this case? > I'm guessing that somewhere there is a: > neg = value < 0; Nope. > Provided all the values are the same size (eg int/s32), in which case: > neg = value; > ... > if ((neg ^ rescale->numerator ^ rescale->denominator) < 0) > will be the desired test.
From: 'Andy Shevchenko' > Sent: 06 December 2024 15:20 ... > ... > > > > * If only one of the rescaler elements or the schan scale is > > > * negative, the combined scale is negative. > > > */ > > > - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { > > > + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) { > > > > That is wrong, the || would also need to be !=. > > Why do you think so? Maybe it's comment(s) that is(are) wrong? The old code certainly negates for each of them - so you can get the double and triple negative cases. So believe the code not the comment. > > > Which will all generate real pile of horrid code. > > (I think the x86 version will stun you.) > > I think your remark is based on something, can you show the output to elaborate > what exactly becomes horrible in this case? Ok it isn't quite as bad as I thought because all the compilers will use xor for the equality test on sign bits. See https://www.godbolt.org/z/qxz5KPcTh So changing ^ to != makes no difference at all. But f3() shows the sort of thing that can happen. Sometimes made worse because the x86 SETcc instruction only set 8bit registers. (Odd since they got added for the 386) David > > > I'm guessing that somewhere there is a: > > neg = value < 0; > > Nope. > > > Provided all the values are the same size (eg int/s32), in which case: > > neg = value; > > ... > > if ((neg ^ rescale->numerator ^ rescale->denominator) < 0) > > will be the desired test. > > -- > With Best Regards, > Andy Shevchenko > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi! 2024-12-06 at 21:13, David Laight wrote: > From: 'Andy Shevchenko' >> Sent: 06 December 2024 15:20 > ... >> ... >> >>>> * If only one of the rescaler elements or the schan scale is >>>> * negative, the combined scale is negative. >>>> */ >>>> - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { >>>> + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) { >>> >>> That is wrong, the || would also need to be !=. >> >> Why do you think so? Maybe it's comment(s) that is(are) wrong? > > The old code certainly negates for each of them - so you can get the double > and triple negative cases. Indeed. And for me, xor is the natural operator for getting to such "oddness" when three or more values needs to be considered. So, I prefer to keep the code as is since a ^ b ^ c is nice and succinct, while anything you try to write using != is going to be convoluted when you have three or more values. > So believe the code not the comment. I claim that the comment is correct. Or at least not incorrect. It might not be complete, but what it does state holds. It does not spell out that the combined scale is positive if both of the rescaler elements and the schan scale are positive. It does not spell out that the combined scale is negative if all three are negative. What it does give you though, is a hint that the whole if () is all about the sign of the combined scale. But yes, the comment could be improved. I expect a fail from this test in iio-test-rescale.c with the new code { .name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative", .numerator = -1000000, .denominator = -8060, .schan_scale_type = IIO_VAL_INT_PLUS_NANO, .schan_val = -10, .schan_val2 = 123456, .expected = "-1240.710106203", }, but the 0-day has been silent. I wonder why? Does it not actually run the tests? There could also be some more tests to try make sure the logic doesn't regress. The first of these should also fail with this patch, while the second should be ok. { .name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative", .numerator = -1, .denominator = -2, .schan_scale_type = IIO_VAL_INT_PLUS_MICRO, .schan_val = 5, .schan_val2 = 1234, .expected = "2.500617", }, { .name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative", .numerator = 1, .denominator = -2, .schan_scale_type = IIO_VAL_INT_PLUS_MICRO, .schan_val = -5, .schan_val2 = 1234, .expected = "2.500617", }, Cheers, Peter
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index b6a46036d5ea..470dd7d41b2a 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -26,7 +26,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, int _val, _val2; s32 rem, rem2; u32 mult; - u32 neg; + bool neg; switch (scale_type) { case IIO_VAL_INT: @@ -95,7 +95,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, * If only one of the rescaler elements or the schan scale is * negative, the combined scale is negative. */ - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) { + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) { if (*val) *val = -*val; else
There are two (non-critical) issues with the code. First of all, the eXclusive OR is not defined for booleans, so boolean to integer promotion is required, Second, the u32 variable is used to keep boolean value, so boolean is converted implicitly to the integer. All these are not needed when variable is defined as boolean to begin with and operations are replaced by simple != and ||. Fixes: 701ee14da95d ("iio: afe: rescale: add INT_PLUS_{MICRO,NANO} support") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/afe/iio-rescale.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)