* drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
@ 2025-01-29 14:02 kernel test robot
2025-01-29 22:21 ` David Laight
0 siblings, 1 reply; 7+ messages in thread
From: kernel test robot @ 2025-01-29 14:02 UTC (permalink / raw)
To: Even Xu
Cc: oe-kbuild-all, linux-kernel, Jiri Kosina, Xinpeng Sun,
Srinivas Pandruvada, Mark Pearson
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid: intel-thc: Add THC DMA interfaces
date: 3 weeks ago
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ovr-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ovr-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/mm.h, ...):
include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false
include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false
vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
287
288 static void release_dma_buffers(struct thc_device *dev,
289 struct thc_dma_configuration *config)
290 {
291 size_t prd_tbls_size = array_size(PRD_TABLE_SIZE, config->prd_tbl_num);
292 unsigned int i;
293
294 if (!config->is_enabled)
295 return;
296
297 for (i = 0; i < config->prd_tbl_num; i++) {
> 298 if (!config->sgls[i] | !config->sgls_nent[i])
299 continue;
300
301 dma_unmap_sg(dev->dev, config->sgls[i],
302 config->sgls_nent[i],
303 config->dir);
304
305 sgl_free(config->sgls[i]);
306 config->sgls[i] = NULL;
307 }
308
309 memset(config->prd_tbls, 0, prd_tbls_size);
310
311 if (config->prd_tbls) {
312 dma_free_coherent(dev->dev, prd_tbls_size, config->prd_tbls,
313 config->prd_tbls_dma_handle);
314 config->prd_tbls = NULL;
315 config->prd_tbls_dma_handle = 0;
316 }
317 }
318
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-01-29 14:02 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y kernel test robot
@ 2025-01-29 22:21 ` David Laight
2025-02-05 2:56 ` Xu, Even
0 siblings, 1 reply; 7+ messages in thread
From: David Laight @ 2025-01-29 22:21 UTC (permalink / raw)
To: kernel test robot
Cc: Even Xu, oe-kbuild-all, linux-kernel, Jiri Kosina, Xinpeng Sun,
Srinivas Pandruvada, Mark Pearson
On Wed, 29 Jan 2025 22:02:59 +0800
kernel test robot <lkp@intel.com> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid: intel-thc: Add THC DMA interfaces
> date: 3 weeks ago
> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ovr-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ovr-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@intel.com/
>
> sparse warnings: (new ones prefixed by >>)
> >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/mm.h, ...):
> include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false
> include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always evaluates to false
>
> vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
...
> 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> 299 continue;
...
If zeros are unlikely the bit-wise 'or' is pretty likely to generate better code
than a logical 'or'.
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-01-29 22:21 ` David Laight
@ 2025-02-05 2:56 ` Xu, Even
2025-02-05 21:56 ` David Laight
0 siblings, 1 reply; 7+ messages in thread
From: Xu, Even @ 2025-02-05 2:56 UTC (permalink / raw)
To: David Laight, lkp
Cc: oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Jiri Kosina, Sun, Xinpeng, Srinivas Pandruvada, Mark Pearson
> -----Original Message-----
> From: David Laight <david.laight.linux@gmail.com>
> Sent: Thursday, January 30, 2025 6:22 AM
> To: lkp <lkp@intel.com>
> Cc: Xu, Even <even.xu@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> <xinpeng.sun@intel.com>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> lenovo@squebb.ca>
> Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> sparse: dubious: !x | !y
>
> On Wed, 29 Jan 2025 22:02:59 +0800
> kernel test robot <lkp@intel.com> wrote:
>
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid:
> intel-thc: Add THC DMA interfaces
> > date: 3 weeks ago
> > config: x86_64-allyesconfig
> > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ov
> > r-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1
> > build):
> > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ov
> > r-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes:
> > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@inte
> > | l.com/
> >
> > sparse warnings: (new ones prefixed by >>)
> > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > >> sparse: dubious: !x | !y
> > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in included file
> (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/mm.h, ...):
> > include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always
> evaluates to false
> > include/linux/page-flags.h:237:46: sparse: sparse: self-comparison
> > always evaluates to false
> >
> > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> ...
> > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > 299 continue;
> ...
>
> If zeros are unlikely the bit-wise 'or' is pretty likely to generate better code than a
> logical 'or'.
>
> David
Good suggestion!
So the code can be optimized to:
for (i = 0; i < config->prd_tbl_num; i++) {
if (config->sgls[i] && config->sgls_nent[i]) {
......
}
}
Will create a patch for this.
Best Regards,
Even Xu
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-02-05 2:56 ` Xu, Even
@ 2025-02-05 21:56 ` David Laight
2025-02-06 2:50 ` Xu, Even
0 siblings, 1 reply; 7+ messages in thread
From: David Laight @ 2025-02-05 21:56 UTC (permalink / raw)
To: Xu, Even
Cc: lkp, oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Jiri Kosina, Sun, Xinpeng, Srinivas Pandruvada, Mark Pearson
On Wed, 5 Feb 2025 02:56:05 +0000
"Xu, Even" <even.xu@intel.com> wrote:
> > -----Original Message-----
> > From: David Laight <david.laight.linux@gmail.com>
> > Sent: Thursday, January 30, 2025 6:22 AM
> > To: lkp <lkp@intel.com>
> > Cc: Xu, Even <even.xu@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> > kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> > <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > lenovo@squebb.ca>
> > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > sparse: dubious: !x | !y
> >
> > On Wed, 29 Jan 2025 22:02:59 +0800
> > kernel test robot <lkp@intel.com> wrote:
> >
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid:
> > intel-thc: Add THC DMA interfaces
> > > date: 3 weeks ago
> > > config: x86_64-allyesconfig
> > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ov
> > > r-lkp@intel.com/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1
> > > build):
> > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFDq4ov
> > > r-lkp@intel.com/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new
> > > version of the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Closes:
> > > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@inte
> > > | l.com/
> > >
> > > sparse warnings: (new ones prefixed by >>)
> > > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > > >> sparse: dubious: !x | !y
> > > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in included file
> > (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/mm.h, ...):
> > > include/linux/page-flags.h:237:46: sparse: sparse: self-comparison always
> > evaluates to false
> > > include/linux/page-flags.h:237:46: sparse: sparse: self-comparison
> > > always evaluates to false
> > >
> > > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> > ...
> > > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > > 299 continue;
> > ...
> >
> > If zeros are unlikely the bit-wise 'or' is pretty likely to generate better code than a
> > logical 'or'.
> >
> > David
>
> Good suggestion!
> So the code can be optimized to:
> for (i = 0; i < config->prd_tbl_num; i++) {
> if (config->sgls[i] && config->sgls_nent[i]) {
> ......
> }
That just adds a level of indentation to the source.
It'll generate much the same code as !x || !y.
Both will generate two conditional branches.
It almost certainly makes in immeasurable difference here, but !x | !y
can be generated using (on x86) the 'sete' instruction to get a 0/1 from
the x == 0 compare (that sets the flags), and then doing an 'or' and a
single jump.
David
> }
>
> Will create a patch for this.
>
> Best Regards,
> Even Xu
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-02-05 21:56 ` David Laight
@ 2025-02-06 2:50 ` Xu, Even
2025-02-06 9:04 ` David Laight
0 siblings, 1 reply; 7+ messages in thread
From: Xu, Even @ 2025-02-06 2:50 UTC (permalink / raw)
To: David Laight
Cc: lkp, oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Jiri Kosina, Sun, Xinpeng, Srinivas Pandruvada, Mark Pearson
> -----Original Message-----
> From: David Laight <david.laight.linux@gmail.com>
> Sent: Thursday, February 6, 2025 5:57 AM
> To: Xu, Even <even.xu@intel.com>
> Cc: lkp <lkp@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> <xinpeng.sun@intel.com>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> lenovo@squebb.ca>
> Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> sparse: dubious: !x | !y
>
> On Wed, 5 Feb 2025 02:56:05 +0000
> "Xu, Even" <even.xu@intel.com> wrote:
>
> > > -----Original Message-----
> > > From: David Laight <david.laight.linux@gmail.com>
> > > Sent: Thursday, January 30, 2025 6:22 AM
> > > To: lkp <lkp@intel.com>
> > > Cc: Xu, Even <even.xu@intel.com>; oe-kbuild-all@lists.linux.dev;
> > > linux- kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun,
> > > Xinpeng <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > > lenovo@squebb.ca>
> > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> sparse:
> > > sparse: dubious: !x | !y
> > >
> > > On Wed, 29 Jan 2025 22:02:59 +0800
> > > kernel test robot <lkp@intel.com> wrote:
> > >
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> > > > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > > > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid:
> > > intel-thc: Add THC DMA interfaces
> > > > date: 3 weeks ago
> > > > config: x86_64-allyesconfig
> > > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFD
> > > > q4ov
> > > > r-lkp@intel.com/config)
> > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a
> > > > W=1
> > > > build):
> > > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFD
> > > > q4ov
> > > > r-lkp@intel.com/reproduce)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a
> > > > new version of the same patch/commit), kindly add following tags
> > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > | Closes:
> > > > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@
> > > > | inte
> > > > | l.com/
> > > >
> > > > sparse warnings: (new ones prefixed by >>)
> > > > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > > > >> sparse: dubious: !x | !y
> > > > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in
> > > > included file
> > > (through include/linux/mmzone.h, include/linux/gfp.h,
> include/linux/mm.h, ...):
> > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > self-comparison always
> > > evaluates to false
> > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > self-comparison always evaluates to false
> > > >
> > > > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> > > ...
> > > > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > > > 299 continue;
> > > ...
> > >
> > > If zeros are unlikely the bit-wise 'or' is pretty likely to generate
> > > better code than a logical 'or'.
> > >
> > > David
> >
> > Good suggestion!
> > So the code can be optimized to:
> > for (i = 0; i < config->prd_tbl_num; i++) {
> > if (config->sgls[i] && config->sgls_nent[i]) {
> > ......
> > }
>
> That just adds a level of indentation to the source.
> It'll generate much the same code as !x || !y.
> Both will generate two conditional branches.
>
> It almost certainly makes in immeasurable difference here, but !x | !y can be
> generated using (on x86) the 'sete' instruction to get a 0/1 from the x == 0
> compare (that sets the flags), and then doing an 'or' and a single jump.
You are right.
I do a simple testing:
83 7d f8 00 cmpl $0x0,-0x8(%rbp)
0f 94 c2 sete %dl
83 7d fc 00 cmpl $0x0,-0x4(%rbp)
0f 94 c0 sete %al
09 d0 or %edx,%eax
84 c0 test %al,%al
74 0f je 1186 <main+0x3d>
Compare to:
83 7d f8 00 cmpl $0x0,-0x8(%rbp)
74 06 je 116f <main+0x26>
83 7d fc 00 cmpl $0x0,-0x4(%rbp)
75 0f jne 117e <main+0x35>
So you suggest I'd better keep the original (!x | !y) code, if just for unlikely Zero comparing?
Thanks!
Best Regards,
Even Xu
>
> David
>
> > }
> >
> > Will create a patch for this.
> >
> > Best Regards,
> > Even Xu
> >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-02-06 2:50 ` Xu, Even
@ 2025-02-06 9:04 ` David Laight
2025-02-07 6:11 ` Xu, Even
0 siblings, 1 reply; 7+ messages in thread
From: David Laight @ 2025-02-06 9:04 UTC (permalink / raw)
To: Xu, Even
Cc: lkp, oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Jiri Kosina, Sun, Xinpeng, Srinivas Pandruvada, Mark Pearson
On Thu, 6 Feb 2025 02:50:17 +0000
"Xu, Even" <even.xu@intel.com> wrote:
> > -----Original Message-----
> > From: David Laight <david.laight.linux@gmail.com>
> > Sent: Thursday, February 6, 2025 5:57 AM
> > To: Xu, Even <even.xu@intel.com>
> > Cc: lkp <lkp@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> > kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> > <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > lenovo@squebb.ca>
> > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > sparse: dubious: !x | !y
> >
> > On Wed, 5 Feb 2025 02:56:05 +0000
> > "Xu, Even" <even.xu@intel.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > Sent: Thursday, January 30, 2025 6:22 AM
> > > > To: lkp <lkp@intel.com>
> > > > Cc: Xu, Even <even.xu@intel.com>; oe-kbuild-all@lists.linux.dev;
> > > > linux- kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun,
> > > > Xinpeng <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > > > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > > > lenovo@squebb.ca>
> > > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> > sparse:
> > > > sparse: dubious: !x | !y
> > > >
> > > > On Wed, 29 Jan 2025 22:02:59 +0800
> > > > kernel test robot <lkp@intel.com> wrote:
> > > >
> > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > master
> > > > > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > > > > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-hid:
> > > > intel-thc: Add THC DMA interfaces
> > > > > date: 3 weeks ago
> > > > > config: x86_64-allyesconfig
> > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFD
> > > > > q4ov
> > > > > r-lkp@intel.com/config)
> > > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a
> > > > > W=1
> > > > > build):
> > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144.eFD
> > > > > q4ov
> > > > > r-lkp@intel.com/reproduce)
> > > > >
> > > > > If you fix the issue in a separate patch/commit (i.e. not just a
> > > > > new version of the same patch/commit), kindly add following tags
> > > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > > | Closes:
> > > > > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-lkp@
> > > > > | inte
> > > > > | l.com/
> > > > >
> > > > > sparse warnings: (new ones prefixed by >>)
> > > > > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > > > > >> sparse: dubious: !x | !y
> > > > > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note: in
> > > > > included file
> > > > (through include/linux/mmzone.h, include/linux/gfp.h,
> > include/linux/mm.h, ...):
> > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > self-comparison always
> > > > evaluates to false
> > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > self-comparison always evaluates to false
> > > > >
> > > > > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> > > > ...
> > > > > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > > > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > > > > 299 continue;
> > > > ...
> > > >
> > > > If zeros are unlikely the bit-wise 'or' is pretty likely to generate
> > > > better code than a logical 'or'.
> > > >
> > > > David
> > >
> > > Good suggestion!
> > > So the code can be optimized to:
> > > for (i = 0; i < config->prd_tbl_num; i++) {
> > > if (config->sgls[i] && config->sgls_nent[i]) {
> > > ......
> > > }
> >
> > That just adds a level of indentation to the source.
> > It'll generate much the same code as !x || !y.
> > Both will generate two conditional branches.
> >
> > It almost certainly makes in immeasurable difference here, but !x | !y can be
> > generated using (on x86) the 'sete' instruction to get a 0/1 from the x == 0
> > compare (that sets the flags), and then doing an 'or' and a single jump.
>
> You are right.
I'm always right (except when I'm not)
> I do a simple testing:
>
> 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> 0f 94 c2 sete %dl
> 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> 0f 94 c0 sete %al
On a modern cpu those two pairs will run in parallel.
> 09 d0 or %edx,%eax
> 84 c0 test %al,%al
The compiler needn't to add the 'test' instruction, but doesn't really like
the fact that the x86 'sete' is only available for 8-bit registers.
> 74 0f je 1186 <main+0x3d>
>
> Compare to:
>
> 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> 74 06 je 116f <main+0x26>
> 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> 75 0f jne 117e <main+0x35>
>
> So you suggest I'd better keep the original (!x | !y) code, if just for unlikely Zero comparing?
Looking at the code I'd guess common case is that both values are non-zero.
So in the normal case both tests are needed.
The branch predictor will (hopefully) get the tests right after the first
iteration, and I suspect the performance of this code isn't that critical.
So while using | might be an optimisation (rather than a mistake)
changing it to || probably doesn't matter.
David
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> >
> > David
> >
> > > }
> > >
> > > Will create a patch for this.
> > >
> > > Best Regards,
> > > Even Xu
> > >
> > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y
2025-02-06 9:04 ` David Laight
@ 2025-02-07 6:11 ` Xu, Even
0 siblings, 0 replies; 7+ messages in thread
From: Xu, Even @ 2025-02-07 6:11 UTC (permalink / raw)
To: David Laight
Cc: lkp, oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Jiri Kosina, Sun, Xinpeng, Srinivas Pandruvada, Mark Pearson
> -----Original Message-----
> From: David Laight <david.laight.linux@gmail.com>
> Sent: Thursday, February 6, 2025 5:05 PM
> To: Xu, Even <even.xu@intel.com>
> Cc: lkp <lkp@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> <xinpeng.sun@intel.com>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> lenovo@squebb.ca>
> Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> sparse: dubious: !x | !y
>
> On Thu, 6 Feb 2025 02:50:17 +0000
> "Xu, Even" <even.xu@intel.com> wrote:
>
> > > -----Original Message-----
> > > From: David Laight <david.laight.linux@gmail.com>
> > > Sent: Thursday, February 6, 2025 5:57 AM
> > > To: Xu, Even <even.xu@intel.com>
> > > Cc: lkp <lkp@intel.com>; oe-kbuild-all@lists.linux.dev; linux-
> > > kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>; Sun, Xinpeng
> > > <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > > lenovo@squebb.ca>
> > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> sparse:
> > > sparse: dubious: !x | !y
> > >
> > > On Wed, 5 Feb 2025 02:56:05 +0000
> > > "Xu, Even" <even.xu@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > Sent: Thursday, January 30, 2025 6:22 AM
> > > > > To: lkp <lkp@intel.com>
> > > > > Cc: Xu, Even <even.xu@intel.com>; oe-kbuild-all@lists.linux.dev;
> > > > > linux- kernel@vger.kernel.org; Jiri Kosina <jikos@kernel.org>;
> > > > > Sun, Xinpeng <xinpeng.sun@intel.com>; Srinivas Pandruvada
> > > > > <srinivas.pandruvada@linux.intel.com>; Mark Pearson <mpearson-
> > > > > lenovo@squebb.ca>
> > > > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> > > sparse:
> > > > > sparse: dubious: !x | !y
> > > > >
> > > > > On Wed, 29 Jan 2025 22:02:59 +0800 kernel test robot
> > > > > <lkp@intel.com> wrote:
> > > > >
> > > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > master
> > > > > > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > > > > > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-
> hid:
> > > > > intel-thc: Add THC DMA interfaces
> > > > > > date: 3 weeks ago
> > > > > > config: x86_64-allyesconfig
> > > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144
> > > > > > .eFD
> > > > > > q4ov
> > > > > > r-lkp@intel.com/config)
> > > > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is
> > > > > > a
> > > > > > W=1
> > > > > > build):
> > > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144
> > > > > > .eFD
> > > > > > q4ov
> > > > > > r-lkp@intel.com/reproduce)
> > > > > >
> > > > > > If you fix the issue in a separate patch/commit (i.e. not just
> > > > > > a new version of the same patch/commit), kindly add following
> > > > > > tags
> > > > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > > > | Closes:
> > > > > > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-
> > > > > > | lkp@
> > > > > > | inte
> > > > > > | l.com/
> > > > > >
> > > > > > sparse warnings: (new ones prefixed by >>)
> > > > > > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > > > > > >> sparse: dubious: !x | !y
> > > > > > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note:
> > > > > > in included file
> > > > > (through include/linux/mmzone.h, include/linux/gfp.h,
> > > include/linux/mm.h, ...):
> > > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > > self-comparison always
> > > > > evaluates to false
> > > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > > self-comparison always evaluates to false
> > > > > >
> > > > > > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> > > > > ...
> > > > > > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > > > > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > > > > > 299 continue;
> > > > > ...
> > > > >
> > > > > If zeros are unlikely the bit-wise 'or' is pretty likely to
> > > > > generate better code than a logical 'or'.
> > > > >
> > > > > David
> > > >
> > > > Good suggestion!
> > > > So the code can be optimized to:
> > > > for (i = 0; i < config->prd_tbl_num; i++) {
> > > > if (config->sgls[i] && config->sgls_nent[i]) {
> > > > ......
> > > > }
> > >
> > > That just adds a level of indentation to the source.
> > > It'll generate much the same code as !x || !y.
> > > Both will generate two conditional branches.
> > >
> > > It almost certainly makes in immeasurable difference here, but !x |
> > > !y can be generated using (on x86) the 'sete' instruction to get a
> > > 0/1 from the x == 0 compare (that sets the flags), and then doing an 'or' and a
> single jump.
> >
> > You are right.
>
> I'm always right (except when I'm not)
>
> > I do a simple testing:
> >
> > 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> > 0f 94 c2 sete %dl
> > 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> > 0f 94 c0 sete %al
>
> On a modern cpu those two pairs will run in parallel.
>
> > 09 d0 or %edx,%eax
> > 84 c0 test %al,%al
>
> The compiler needn't to add the 'test' instruction, but doesn't really like the fact
> that the x86 'sete' is only available for 8-bit registers.
>
> > 74 0f je 1186 <main+0x3d>
> >
> > Compare to:
> >
> > 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> > 74 06 je 116f <main+0x26>
> > 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> > 75 0f jne 117e <main+0x35>
> >
> > So you suggest I'd better keep the original (!x | !y) code, if just for unlikely Zero
> comparing?
>
> Looking at the code I'd guess common case is that both values are non-zero.
> So in the normal case both tests are needed.
>
> The branch predictor will (hopefully) get the tests right after the first iteration,
> and I suspect the performance of this code isn't that critical.
>
> So while using | might be an optimisation (rather than a mistake) changing it to ||
> probably doesn't matter.
>
> David
>
Understood.
Thanks for your suggestions and so many details!
Then I prefer to change back to "||" to make it more readable and avoid miss understanding.
Thanks!
Best Regards,
Even
>
> >
> > Thanks!
> >
> > Best Regards,
> > Even Xu
> >
> > >
> > > David
> > >
> > > > }
> > > >
> > > > Will create a patch for this.
> > > >
> > > > Best Regards,
> > > > Even Xu
> > > >
> > > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-02-07 6:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-29 14:02 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y kernel test robot
2025-01-29 22:21 ` David Laight
2025-02-05 2:56 ` Xu, Even
2025-02-05 21:56 ` David Laight
2025-02-06 2:50 ` Xu, Even
2025-02-06 9:04 ` David Laight
2025-02-07 6:11 ` Xu, Even
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox