Remove unnecessary if in __secs_to_tm

Submitted by Mattias Andrée on Feb. 28, 2021, 3:09 p.m.

Details

Message ID 20210228150912.1532943-1-maandree@kth.se
State New
Series "Remove unnecessary if in __secs_to_tm"
Headers show

Commit Message

Mattias Andrée Feb. 28, 2021, 3:09 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

Rich Felker Feb. 28, 2021, 5:06 p.m.
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
Mattias Andrée Feb. 28, 2021, 5:24 p.m.
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 -=.
Rich Felker Feb. 28, 2021, 5:34 p.m.
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