fmpz_zero() assigns zero twice

[krisk0]

fmpz_zero() when called with a big number assigns zero twice: once in _fmpz_demote() and once by itself.

I offer a patch, to fix the problem (can't sleep at night when my program works slower than it could)

--- fmpz.h      2014-04-01 03:48:29.000000000 +0400
+++ fmpz-m.h    2014-04-17 18:36:53.312158379 +0400
@@ -270,8 +270,9 @@
 static __inline__
 void fmpz_zero(fmpz_t f)
 {
-   _fmpz_demote(f);
+   if (COEFF_IS_MPZ(*f)) 
+       _fmpz_clear_mpz(*f);
 }

[wbhart]

This should be ok. However we should also add a comment to note we've done
that, since if fmpz_demote changes, this will have to as well.

Bill.

On 17 April 2014 16:43, Денис Крыськов (Denis Kryskov) <
notifications@github.com> wrote:

fmpz_zero() when called with a big number assigns zero twice: once in
_fmpz_demote() and once by itself.

I offer a patch, to fix the problem (can't sleep at night when my program
works slower than it could)

--- fmpz.h 2014-04-01 03:48:29.000000000 +0400
+++ fmpz-m.h 2014-04-17 18:36:53.312158379 +0400
@@ -270,8 +270,9 @@
static inline
void fmpz_zero(fmpz_t f)
{

• _fmpz_demote(f);
• if (COEFF_IS_MPZ(*f))

•   _fmpz_clear_mpz(*f);
  

  }

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/64
.

[krisk0]

I am not sure a comment is needed, since git is tracking and tests are testing.

But you are a boss...

--- fmpz.h      2014-04-01 03:48:29.000000000 +0400
+++ fmpz-m.h    2014-04-17 19:29:52.206618602 +0400
@@ -269,13 +269,16 @@

 static __inline__
 void fmpz_zero(fmpz_t f)
+/* Warning: if _fmpz_demote() changes, this subroutine might need changes,too */
 {
-   _fmpz_demote(f);
-   (*f) = WORD(0);
+   if (COEFF_IS_MPZ(*f)) 
+       _fmpz_clear_mpz(*f);
+   *f = WORD(0);
 }

 static __inline__ 
 void fmpz_one(fmpz_t f)
+/* Warning: if _fmpz_demote() changes, this subroutine might need changes,too */
 {
     if (COEFF_IS_MPZ(*f)) 
     {

I've added comment twice since same warning applies to fmpz_one()

[wbhart]

I really meant to add a warning to _fmpz_demote that if it changes, the
person would also need to change fmpz_one and fmpz_zero.

People certainly won't go back through the entire git history to see what
needs to be done if this function is changed.

Bill.

On 17 April 2014 17:33, Денис Крыськов (Denis Kryskov) <
notifications@github.com> wrote:

I am not sure a comment is needed, since git is tracking and tests are
testing.

But you are a boss...

--- fmpz.h 2014-04-01 03:48:29.000000000 +0400
+++ fmpz-m.h 2014-04-17 19:29:52.206618602 +0400
@@ -269,13 +269,16 @@

static inline
void fmpz_zero(fmpz_t f)
+/* Warning: if _fmpz_demote() changes, this subroutine might need changes,too */
{

• _fmpz_demote(f);
• (*f) = WORD(0);
• if (COEFF_IS_MPZ(*f))

•   _fmpz_clear_mpz(*f);
  

• *f = WORD(0);
  }

static inline
void fmpz_one(fmpz_t f)
+/* Warning: if _fmpz_demote() changes, this subroutine might need changes,too _/
{
if (COEFF_IS_MPZ(_f))
{

I've added comment twice since same warning applies to fmpz_one()

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/64#issuecomment-40727531
.

[krisk0]

--- fmpz.h      2014-04-01 03:48:29.000000000 +0400
+++ fmpz-m.h    2014-04-17 20:03:36.187428292 +0400
@@ -83,6 +83,10 @@

 static __inline__
 void _fmpz_demote(fmpz_t f)
+/* Warning: 
+ *  if this procedure changes, then other procedures including fmpz_one() and 
+ *   fmpz_zero() might need changes, too
+*/
 {
     if (COEFF_IS_MPZ(*f)) 
     {
@@ -270,8 +274,9 @@
 static __inline__
 void fmpz_zero(fmpz_t f)
 {
-   _fmpz_demote(f);
-   (*f) = WORD(0);
+   if (COEFF_IS_MPZ(*f)) 
+       _fmpz_clear_mpz(*f);
+   *f = WORD(0);
 }

 static __inline__ 

[fredrik-johansson]

Since the functions are inlined, won't the compiler easily be able to remove the duplicate assignments anyway? The actual assembly output would be interesting.

[krisk0]

No, compiler did not remove it. GCC 4.7, CFLAGS: -O2 -march=native (that I use by default).

movq $0x0 is inside _fmpz_demote() and main()

C code:

#include <flint/fmpz_mat.h>
int main()
 {
  fmpz_mat_t m;
  fmpz_mat_read( m );
  fmpz_zero( fmpz_mat_entry( m, 1, 2) );
  fmpz_zero( fmpz_mat_entry( m, 1, 3) );
  fmpz_zero( fmpz_mat_entry( m, 1, 4) );
  fmpz_zero( fmpz_mat_entry( m, 1, 5) );
  fmpz_zero( fmpz_mat_entry( m, 1, 6) );
  fmpz_mat_print( m );
 }

asm code:

fmpz_zero_asm.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <_fmpz_demote>:
   0:   53                      push   %rbx
   1:   48 89 fb                mov    %rdi,%rbx
   4:   48 8b 3f                mov    (%rdi),%rdi
   7:   48 89 f8                mov    %rdi,%rax
   a:   48 c1 f8 3e             sar    $0x3e,%rax
   e:   48 83 f8 01             cmp    $0x1,%rax
  12:   74 02                   je     16 <_fmpz_demote+0x16>
  14:   5b                      pop    %rbx
  15:   c3                      retq   
  16:   e8 00 00 00 00          callq  1b <_fmpz_demote+0x1b>
  1b:   48 c7 03 00 00 00 00    movq   $0x0,(%rbx)
  22:   eb f0                   jmp    14 <_fmpz_demote+0x14>

Disassembly of section .text.startup:

0000000000000000 <main>:
   0:   53                      push   %rbx
   1:   48 83 ec 20             sub    $0x20,%rsp
   5:   48 8b 3d 00 00 00 00    mov    0x0(%rip),%rdi        # c <main+0xc>
   c:   48 89 e6                mov    %rsp,%rsi
   f:   e8 00 00 00 00          callq  14 <main+0x14>
  14:   48 8b 44 24 18          mov    0x18(%rsp),%rax
  19:   48 8b 58 08             mov    0x8(%rax),%rbx
  1d:   48 8d 7b 10             lea    0x10(%rbx),%rdi
  21:   e8 00 00 00 00          callq  26 <main+0x26>
  26:   48 8b 44 24 18          mov    0x18(%rsp),%rax
  2b:   48 c7 43 10 00 00 00    movq   $0x0,0x10(%rbx)
  32:   00 
  33:   48 8b 58 08             mov    0x8(%rax),%rbx
  37:   48 8d 7b 18             lea    0x18(%rbx),%rdi
  3b:   e8 00 00 00 00          callq  40 <main+0x40>
  40:   48 8b 44 24 18          mov    0x18(%rsp),%rax
  45:   48 c7 43 18 00 00 00    movq   $0x0,0x18(%rbx)
  4c:   00 
  4d:   48 8b 58 08             mov    0x8(%rax),%rbx
  51:   48 8d 7b 20             lea    0x20(%rbx),%rdi
  55:   e8 00 00 00 00          callq  5a <main+0x5a>
  5a:   48 8b 44 24 18          mov    0x18(%rsp),%rax
  5f:   48 c7 43 20 00 00 00    movq   $0x0,0x20(%rbx)
  66:   00 
  67:   48 8b 58 08             mov    0x8(%rax),%rbx
  6b:   48 8d 7b 28             lea    0x28(%rbx),%rdi
  6f:   e8 00 00 00 00          callq  74 <main+0x74>
  74:   48 8b 44 24 18          mov    0x18(%rsp),%rax
  79:   48 c7 43 28 00 00 00    movq   $0x0,0x28(%rbx)
  80:   00 
  81:   48 8b 58 08             mov    0x8(%rax),%rbx
  85:   48 8d 7b 30             lea    0x30(%rbx),%rdi
  89:   e8 00 00 00 00          callq  8e <main+0x8e>
  8e:   48 8b 3d 00 00 00 00    mov    0x0(%rip),%rdi        # 95 <main+0x95>
  95:   48 89 e6                mov    %rsp,%rsi
  98:   48 c7 43 30 00 00 00    movq   $0x0,0x30(%rbx)
  9f:   00 
  a0:   e8 00 00 00 00          callq  a5 <main+0xa5>
  a5:   48 83 c4 20             add    $0x20,%rsp
  a9:   5b                      pop    %rbx
  aa:   c3                      retq   

[wbhart]

Merged.