From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 3826 invoked from network); 28 Feb 2021 17:34:34 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 28 Feb 2021 17:34:34 -0000 Received: (qmail 25622 invoked by uid 550); 28 Feb 2021 17:34:32 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 25604 invoked from network); 28 Feb 2021 17:34:31 -0000 Date: Sun, 28 Feb 2021 12:34:19 -0500 From: Rich Felker To: Mattias =?utf-8?Q?Andr=C3=A9e?= Cc: musl@lists.openwall.com Message-ID: <20210228173419.GG32655@brightrain.aerifal.cx> References: <20210228150912.1532943-1-maandree@kth.se> <20210228170614.GF32655@brightrain.aerifal.cx> <20210228182445.66f1de08.maandree@kth.se> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210228182445.66f1de08.maandree@kth.se> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] Remove unnecessary if in __secs_to_tm 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 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