Re: [musl] [PATCH] Remove unnecessary if in __secs_to_tm

[Rich Felker <dalias@libc.org>]

On Sun, Feb 28, 2021 at 06:24:45PM +0100, Mattias Andrée wrote:
> On Sun, 28 Feb 2021 12:06:15 -0500
> Rich Felker <dalias@libc.org> wrote:
> 
> > On Sun, Feb 28, 2021 at 04:09:12PM +0100, Mattias Andrée wrote:
> > > Since years divisible by 100 but not by 400 are not leap years,
> > > q_cycles can at most be 24 (DAYS_PER_100Y / DAYS_PER_4Y == 24).
> > > ---
> > >  src/time/__secs_to_tm.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/src/time/__secs_to_tm.c b/src/time/__secs_to_tm.c
> > > index 093d9021..2d0c0b2c 100644
> > > --- a/src/time/__secs_to_tm.c
> > > +++ b/src/time/__secs_to_tm.c
> > > @@ -44,8 +44,7 @@ int __secs_to_tm(long long t, struct tm *tm)
> > >  	remdays -= c_cycles * DAYS_PER_100Y;
> > >  
> > >  	q_cycles = remdays / DAYS_PER_4Y;
> > > -	if (q_cycles == 25) q_cycles--;
> > > -	remdays -= q_cycles * DAYS_PER_4Y;
> > > +	remdays %= DAYS_PER_4Y;
> > >  
> > >  	remyears = remdays / 365;
> > >  	if (remyears == 4) remyears--;  
> > 
> > I think you're right about the condition being impossible -- it looks
> > like the error in thinking was that, while 400Y and 4Y are strictly
> > larger than 4*100Y and 4*1Y respectively, 100Y is smaller than 25*4Y.
> > 
> > However, changing the -= to %= is not desirable. The point of the -=
> > has nothing to do with the edge case that can't happen; it's to avoid
> > a modulo operation. Since the divisor is a constant though maybe the
> > compiler can generate the same code for both, anyway..?
> > 
> > Rich
> 
> For x86_64 `remdays %= DAYS_PER_4Y` just becomes a move.
> 
> 	divmod in
> 
> 		int r = 52, q;
> 		void divmod(void)
> 		{
> 		        q = r / 111;
> 		        r %= 111;
> 		}
> 
> 	becomes
> 
> 		movl	r(%rip), %eax
> 		movl	$111, %ecx
> 		cltd
> 		idivl	%ecx
> 		movl	%eax, q(%rip)
> 		movl	%edx, r(%rip)
> 		ret
> 
> `remdays -= q_cycles * DAYS_PER_4Y;` on the other hand
> becomes a move, a multiplication, and an addition.
> 
> 	divmod in
> 
> 		int r = 52, q;
> 		void divmod(void)
> 		{
> 		        q = r / 111;
> 		        r -= q * 111;
> 		}
> 
> 	becomes
> 
> 		movl	r(%rip), %eax
> 		movl	$111, %ecx
> 		cltd
> 		idivl	%ecx
> 		movl	%eax, q(%rip)
> 		imull	$-111, %eax, %eax
> 		addl	r(%rip), %eax
> 		movl	%eax, r(%rip)
> 		ret
> 
> So I would say %= is the better option, at least for x86_64.
> 
> Of course, if you prefer, I will change it to use -=.

It's an unrelated change, so if it should be made it should be done as
a different commit, and in all the places not just arbitrarily in one
of them. But the above analysis is probably not indicative. You're
dividing by a variable, in which case on x86_64 idiv gets used and
there's a remainder available for free. But in the code here all the
divisions are by constants and should cause the compiler to emit code
using only multiplies.

(Note: this may not currently be the case with -Os, which is one big
reason we should be dropping -Os and instead tuning -O2 to behave
better, which is a longstanding agenda item).

Also, if these were actual div/mod operations by a variable, the
interesting case is not archs with an instruction that produces the
remainder for free, but ones where two separate operations are
required or where long division in software is required.

Rich