Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse: sparse: dubious: !x | !y

[David Laight <david.laight.linux@gmail.com>]

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
> > >
> > >  
>