./note006.html

ToC

6. プログラムを楽に書く(その2)。(2009/6/14)

これは某君にみせてもらったプログラムから。前節のプログラムを書いた人とは別人だが、問題点は全く同じである。

  int CTRL_write_nhib(CTRLinfo *c[NCHIP], int nc, int module, UINT64 addr, const void *data[NCHIP], int len) 
  {
    UINT64 *p[NCHIP];
    UINT64 header;
    int devid[NCHIP];
    int i, j, k, size;
    Hib *hib[NCHIP];
    int s, t, last, x;
  
    for(i=0;i<nc;i++){
      devid[i] = c[i]->hib_id;
      hib[i] = c[i]->h;
      p[i] = (UINT64*)data[i];
    }
    //s = (hib[0]->piowbuf_bytes>>3)-1;
    s = 16;   // 16LW send
    t = 256;  // 256LW swap
  
    if(c[0]->bcastflag){
      header = ((UINT64)len << 40) | ((UINT64)1 << 39) | ((UINT64)module<<36) | addr;
    } else {
      header = ((UINT64)len << 40) | ((UINT64)module << 36) | addr; //For new hic header KK
    }
    size = len+1;
    
    last=0;
    for (i = 0; i < size; i += s) {
      if (i + s > size) {
        s = size - i;
        last=1;
      }
      if(i%t==0 && i!=0){
        hib_mem_writeMC(devid[0], hib[0]->dma0misc,
                      (1<<(hib[0]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[0]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[1], hib[1]->dma0misc,
                      (1<<(hib[1]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[1]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[2], hib[2]->dma0misc,
                      (1<<(hib[2]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[2]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[3], hib[3]->dma0misc,
                      (1<<(hib[3]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[3]->dma0misc_sram_wcnt_bit)));
        x=0;
      }
  
      if(i==0){
        x = 1;
        start = rdtsc2();
        hib[0]->piow_dblbuf[0] = header;
        hib[1]->piow_dblbuf[0] = header;
        hib[2]->piow_dblbuf[0] = header;
        hib[3]->piow_dblbuf[0] = header;
        for (j = 0; j < s-1; j++){
        //hib[0]->piow_dblbuf[x+j] = p[0][i + j];
        __builtin_ia32_movntq(&(hib[0]->piow_dblbuf[x+j]), p[0][i + j]);
        }
        for (j = 0; j < s-1; j++) {
        //hib[1]->piow_dblbuf[x+j] = p[1][i + j];
        __builtin_ia32_movntq(&(hib[1]->piow_dblbuf[x+j]), p[1][i + j]);
        }
        for (j = 0; j < s-1; j++) {
        //hib[2]->piow_dblbuf[x+j] = p[2][i + j];
        __builtin_ia32_movntq(&(hib[2]->piow_dblbuf[x+j]), p[2][i + j]);
        }
        for (j = 0; j < s-1; j++) {
        //hib[3]->piow_dblbuf[x+j] = p[3][i + j];
        __builtin_ia32_movntq(&(hib[3]->piow_dblbuf[x+j]), p[3][i + j]);
        }
  
        x+=s-1;
      } else {
        for (j = 0; j < s; j++){
        //hib[0]->piow_dblbuf[x+j] = p[0][i + j -1];
        __builtin_ia32_movntq(&(hib[0]->piow_dblbuf[x+j]), p[0][i + j -1]);
        }
  
        for (j = 0; j < s; j++){
        //hib[1]->piow_dblbuf[x+j] = p[1][i + j -1];
        __builtin_ia32_movntq(&(hib[1]->piow_dblbuf[x+j]), p[1][i + j -1]);
        }
        for (j = 0; j < s; j++){
        //hib[2]->piow_dblbuf[x+j] = p[2][i + j -1];
        __builtin_ia32_movntq(&(hib[2]->piow_dblbuf[x+j]), p[2][i + j -1]);
        }
        for (j = 0; j < s; j++){
        //hib[3]->piow_dblbuf[x+j] = p[3][i + j -1];
        __builtin_ia32_movntq(&(hib[3]->piow_dblbuf[x+j]), p[3][i + j -1]);
        }
        x+=s;
      }
          
      if(last==1){
        hib_mem_writeMC(devid[0], hib[0]->dma0misc,
                      (1<<(hib[0]->dma0misc_swap_sram_bit)) |
                      (s<<(hib[0]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[1], hib[1]->dma0misc,
                      (1<<(hib[1]->dma0misc_swap_sram_bit)) |
                      (s<<(hib[1]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[2], hib[2]->dma0misc,
                      (1<<(hib[2]->dma0misc_swap_sram_bit)) |
                      (s<<(hib[2]->dma0misc_sram_wcnt_bit)));
        hib_mem_writeMC(devid[3], hib[3]->dma0misc,
                      (1<<(hib[3]->dma0misc_swap_sram_bit)) |
                      (s<<(hib[3]->dma0misc_sram_wcnt_bit)));
        x=0;
      }
    }
  }

このプログラムをみてなんだかわかる人というのはそうそうはいないと思うが、これは GRAPE-DR カードの4チャネルのホストリンクに、それぞれコマンド1語+データ 1024語(ここでは1語8バイト)を送るプログラムである。ハードウェアのほうで、ダブルバッファをもっているので、例えば 256語毎にバッファを切換える必要があり、それは hib_mem_writeMC を呼ぶことで行われる。バッファ自体はメモリマップされている。

しかし、中身がわからなくてもよろしくないところがいくつもあるのはわかるであろう。まず、

     hib_mem_writeMC(devid[0], hib[0]->dma0misc,
                      (1<<(hib[0]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[0]->dma0misc_sram_wcnt_bit)));
      hib_mem_writeMC(devid[1], hib[1]->dma0misc,
                      (1<<(hib[1]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[1]->dma0misc_sram_wcnt_bit)));
      hib_mem_writeMC(devid[2], hib[2]->dma0misc,
                      (1<<(hib[2]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[2]->dma0misc_sram_wcnt_bit)));
      hib_mem_writeMC(devid[3], hib[3]->dma0misc,
                      (1<<(hib[3]->dma0misc_swap_sram_bit)) |
                      (t<<(hib[3]->dma0misc_sram_wcnt_bit)));

をみてみよう。これは、関数呼び出しであり、これを書いた人にはこの関数呼び出しは長い時間がかかる、ということがわかっていた。そうすると、

      for(k=0;k<nc;k++){
          hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                          (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                          (t<<(hib[k]->dma0misc_sram_wcnt_bit)));
      }

とループにしない理由は特に思い付くことができない。これは、その下にある

      hib[0]->piow_dblbuf[0] = header;
      hib[1]->piow_dblbuf[0] = header;
      hib[2]->piow_dblbuf[0] = header;
      hib[3]->piow_dblbuf[0] = header;
      for (j = 0; j < s-1; j++){
        //hib[0]->piow_dblbuf[x+j] = p[0][i + j];
        __builtin_ia32_movntq(&(hib[0]->piow_dblbuf[x+j]), p[0][i + j]);
      }
      for (j = 0; j < s-1; j++) {
        //hib[1]->piow_dblbuf[x+j] = p[1][i + j];
        __builtin_ia32_movntq(&(hib[1]->piow_dblbuf[x+j]), p[1][i + j]);
      }
      for (j = 0; j < s-1; j++) {
        //hib[2]->piow_dblbuf[x+j] = p[2][i + j];
        __builtin_ia32_movntq(&(hib[2]->piow_dblbuf[x+j]), p[2][i + j]);
      }
      for (j = 0; j < s-1; j++) {
        //hib[3]->piow_dblbuf[x+j] = p[3][i + j];
        __builtin_ia32_movntq(&(hib[3]->piow_dblbuf[x+j]), p[3][i + j]);
      }

も同様で、

      for(k=0;k<nc;k++){
          hib[k]->piow_dblbuf[k] = header;
          for (j = 0; j < s-1; j++){
              __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j]);
          }
      }

ですむ。さらに2箇所ある同じようなところを直すと

  int CTRL_write_nhib(CTRLinfo *c[NCHIP], int nc, int module, UINT64 addr, const void *data[NCHIP], int len) 
  {
    UINT64 *p[NCHIP];
    UINT64 header;
    int devid[NCHIP];
    int i, j, k, size;
    Hib *hib[NCHIP];
    int s, t, last, x;
  
    for(i=0;i<nc;i++){
      devid[i] = c[i]->hib_id;
      hib[i] = c[i]->h;
      p[i] = (UINT64*)data[i];
    }
    //s = (hib[0]->piowbuf_bytes>>3)-1;
    s = 16;   // 16LW send
    t = 256;  // 256LW swap
  
    if(c[0]->bcastflag){
      header = ((UINT64)len << 40) | ((UINT64)1 << 39) | ((UINT64)module<<36) | addr;
    } else {
      header = ((UINT64)len << 40) | ((UINT64)module << 36) | addr; //For new hic header KK
    }
    size = len+1;
    
    last=0;
    for (i = 0; i < size; i += s) {
      if (i + s > size) {
        s = size - i;
        last=1;
      }
      if(i%t==0 && i!=0){
        for(k=0;k<nc;k++){
            hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                          (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                          (t<<(hib[k]->dma0misc_sram_wcnt_bit)));
        }
        x=0;
      }
  
      if(i==0){
        x = 1;
        for(k=0;k<nc;k++){
            hib[k]->piow_dblbuf[k] = header;
            for (j = 0; j < s-1; j++){
                __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j]);
            }
        }
  
        x+=s-1;
      } else {
        for(k=0;k<nc;k++){
            for (j = 0; j < s; j++){
                __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j-1]);
            }
        }
        x+=s;
      }
          
      if(last==1){
        for(k=0;k<nc;k++){
            hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                          (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                          (s<<(hib[k]->dma0misc_sram_wcnt_bit)));
        }
        x=0;
      }
    }
  }

さて、だいぶコードが短くなったので、構造が少しはわかりやすくなっている。こうなると、 __builtin_ia32_movntq を使っている2つのループ、および hib_mem_writeMC を使っている2つのループが、それぞれ殆ど同じ構造であることがわかる。 __builtin_ia32_movntq はループを回る回数と配列の添字が微妙に違い、 hib_mem_writeMC は引数の1つの変数名が違う。まず、 __builtin_ia32_movntq をみてみよう。これは i==0 かどうかでどっちになるかが変わるので、

    int jstart;
    ....
    if(i==0){
        x = 1;
        for(k=0;k<nc;k++) hib[k]->piow_dblbuf[k] = header;
        jstart = 1;
    }else{
        jstart = 0;
    } 
    for(k=0;k<nc;k++){
        for (j = jstart; j < s; j++){
            __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j-1]);
        }
    }
    if (i==0){
        x+=s-1;
    }else{
        x+=s;
    }

でよい。ただ、 i==0 の時の扱いがなんだか変な気がするであろう。 hib_mem_writeMC はもうちょっと問題である。上のほうは、 i%==0 でなおかつ i!=0 の時に呼ばれ、下のほうはともかく最後に呼ばれる。この動作を良く考えると、実は、 x==t の時か last=1 の時でよいことがわかる。つまり、下のほうを

    if((last==1)|| (x==t)){
      for(k=0;k<nc;k++){
          hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                          (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                          (x<<(hib[k]->dma0misc_sram_wcnt_bit)));
      }
      x=0;
    }

とすれば上は不要である。なお、ここまでくると最初のプログラムにばバグがあったことがわかる。 s と t が使われていた引数は、 t のほうは良いが s のほうは正しくない。ここで渡すべき値は最後にこの関数を呼んでから書いた語数であり、それは x であって s ではないからである。

これで、

  int CTRL_write_nhib(CTRLinfo *c[NCHIP], int nc, int module, UINT64 addr, const void *data[NCHIP], int len) 
  {
    UINT64 *p[NCHIP];
    UINT64 header;
    int devid[NCHIP];
    int i, j, k, size;
    Hib *hib[NCHIP];
    int s, t, last, x;
    int jstart;
    for(i=0;i<nc;i++){
      devid[i] = c[i]->hib_id;
      hib[i] = c[i]->h;
      p[i] = (UINT64*)data[i];
    }
    //s = (hib[0]->piowbuf_bytes>>3)-1;
    s = 16;   // 16LW send
    t = 256;  // 256LW swap
  
    if(c[0]->bcastflag){
      header = ((UINT64)len << 40) | ((UINT64)1 << 39) | ((UINT64)module<<36) | addr;
    } else {
      header = ((UINT64)len << 40) | ((UINT64)module << 36) | addr; //For new hic header KK
    }
    size = len+1;
    
    last=0;
    x=0;
    for (i = 0; i < size; i += s) {
      if (i + s > size) {
        s = size - i;
        last=1;
      }
      if(i==0){
          x = 1;
          for(k=0;k<nc;k++) hib[k]->piow_dblbuf[k] = header;
          jstart = 1;
      }else{
          jstart = 0;
      } 
      for(k=0;k<nc;k++){
          for (j = jstart; j < s; j++){
              __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j-1]);
          }
      }
      if (i==0){
          x+=s-1;
      }else{
          x+=s;
      }
      if((last==1)|| (x==t)){
        for(k=0;k<nc;k++){
            hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                          (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                          (x<<(hib[k]->dma0misc_sram_wcnt_bit)));
        }
        x=0;
      }
    }
  }

さて、良く考えてみると、 i==0 の時にしか実行しないものはループの外でよい。とすると、

  int CTRL_write_nhib(CTRLinfo *c[NCHIP], int nc, int module, UINT64 addr, const void *data[NCHIP], int len) 
  {
    UINT64 *p[NCHIP];
    UINT64 header;
    int devid[NCHIP];
    int i, j, k, size;
    Hib *hib[NCHIP];
    int s, t, last, x;
    int jstart;
    for(i=0;i<nc;i++){
      devid[i] = c[i]->hib_id;
      hib[i] = c[i]->h;
      p[i] = (UINT64*)data[i];
    }
    //s = (hib[0]->piowbuf_bytes>>3)-1;
    s = 16;   // 16LW send
    t = 256;  // 256LW swap
  
    if(c[0]->bcastflag){
      header = ((UINT64)len << 40) | ((UINT64)1 << 39) | ((UINT64)module<<36) | addr;
    } else {
      header = ((UINT64)len << 40) | ((UINT64)module << 36) | addr; //For new hic header KK
    }
    size = len+1;
    
    last=0;
    for(k=0;k<nc;k++) hib[k]->piow_dblbuf[k] = header;
    x=1;
    jstart=1;
    for (i = 0; i < size; i += s) {
      if (i + s > size) {
        s = size - i;
        last=1;
      }
      for(k=0;k<nc;k++){
          for (j = jstart; j < s; j++){
              __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[x+j]), p[0][i + j-1]);
          }
      }
      x+=s-jstart;
      jstart=0;
      if((last==1)|| (x==t)){
          for(k=0;k<nc;k++){
              hib_mem_writeMC(devid[0], hib[k]->dma0misc,
                                  (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                            (x<<(hib[k]->dma0misc_sram_wcnt_bit)));
          }
          x=0;
       }
    }
  }

となる。なお、私が最初に書き直したプログラムは

  int CTRL_write_nhib(CTRLinfo *c[NCHIP], int nc, int module,
                    UINT64 addr, const void *data[NCHIP], int len) 
  {
      UINT64 *p[NCHIP];
      UINT64 header, start, swap, trans, b, e;
      int devid[NCHIP];
      int i, j, k, size;
      Hib *hib[NCHIP];
      int s, t;
      int jstart,iblk;
  
      for(i=0;i<nc;i++){
        devid[i] = c[i]->hib_id;
        hib[i] = c[i]->h;
        p[i] = (UINT64*)data[i];
      }
      //s = (hib[0]->piowbuf_bytes>>3)-1;
      s = 16;   // 16LW send
      t = 256;  // 256LW swap
    
      if(c[0]->bcastflag){
        header = ((UINT64)len << 40) | ((UINT64)1 << 39) | ((UINT64)module<<36) | addr;
      } else {
        header = ((UINT64)len << 40) | ((UINT64)module << 36) | addr; //For new hic header KK
      }
      size = len+1;
      jstart = 1;
      for (k=0;k<4;k++)hib[k]->piow_dblbuf[0] = header;
      for(iblk =0;iblk < size; iblk +=t){
        int  ibsend;
        ibsend = t;
        if (iblk+ibsend > size) ibsend = size - iblk;
        for(i=0;i<ibsend;i+= s){
            int isend = s;
            if (isend + i > ibsend) isend = ibsend - i;
            for (k=0;k<4;k++){
                for (j = jstart; j < isend; j++){
                    __builtin_ia32_movntq(&(hib[k]->piow_dblbuf[i+j]),
                                          p[k][iblk+i+j-1]);
                }
            }
            jstart = 0; // should be 1 only for the first time
        }
        for(k=0;k<4;k++){
            hib_mem_writeMC(devid[k], hib[k]->dma0misc,
                            (1<<(hib[k]->dma0misc_swap_sram_bit)) |
                            (ibsend<<(hib[k]->dma0misc_sram_wcnt_bit)));
        }
      }
  }

上のほうのコードでは s 語送るのを繰り返し、 t 語送ったら hib_mem_writeMC を呼ぶ、という構造にしているが、下のコードでは t 語送ってhib_mem_writeMC を呼ぶ、というループがあって、そのループの中で s 語づつ送る、という構造になっている。行数もほとんど同じなのでどちらでもよいが、この場合に限っていうと下のほうがよい。というのは、上のほうは間違っているからで、

    if (i + s > size) {
      s = size - i;
      last=1;
    }

は

    if (i + s >= size) {
      s = size - i;
      last=1;
    }

でなければならない。そうでないと、 i+s==size の時に last=0 のままなので、hib_mem_writeMC が実行されず、バッファに送ったものが認識されないのである。下の形では、「t語(端数がでる時はこれより少ない)送る」というシーケンスの最後で必ず hib_mem_writeMC が実行されるので、このようないわゆる fencepost error と言われる条件判断ミスが起こりにくい。

繰り返しに対して、それをプログラム上で人間にわかりやすい形で表現することは意外に重要なことである。例えば、

  
    i=0;
  LOOP:
    a[i]=b[i]+c[i];
    ....
    ....
    ....
    ....
    ....
    ....
    ....
    ....
    i = i + 1;
    if (i < n) goto LOOP

(すみません、Cでラベルや goto 文をあまり使わないので、正しいかどうか不明)と書くよりも

  
    i=0;
    while(i<n){
      a[i]=b[i]+c[i];
      ....
      ....
      ....
      ....
      ....
      ....
      ....
      ....
      i = i + 1;
    }         Mail/inbox/

が、さらに

    for(i=0;i<n;i++){
      a[i]=b[i]+c[i];
      ....
      ....
      ....
      ....
      ....
      ....
      ....
      ....
    }

のほうが繰り返しであるとわかりやすい。また、これに関する限り、 Fortran77 の

   do i=1,n  
      a(i)=b(i)+c(i)
      ....
      ....
      ....
      ....
      ....
      ....
      ....
      ....
    enddo

のほうがさらにわかりやすいし、 F90 以降の配列表記で

      a=b+c
      ....
      ....
      ....
      ....
      ....
      ....
      ....
      ....

ですめばそのほうがもっとよいに決まっている。配列表記の問題は、インデックスのレンジを式やブロック全体について指定する良い方法がないので、配列の一部に対する操作だと極めて冗長で読み難い表現になることである。これは適当な文法拡張やマクロ定義でなんとかできそうなものだが。

なお、C 言語だと相変わらず max, min といったものが標準では提供されてないような気がするので、(浮動小数点の fmax, fmin はある)

        ibsend = min(t, size-iblk)

と書けるべきところを

        ibsend = t;
        if (iblk+ibsend > size) ibsend = size - iblk;

という煩雑な記法になっているのでわかりにくい。多少でも大規模なプログラム開発をするなら、max, min といった必ず使うものはインライン関数やマクロで用意して、煩雑な記法を整理するべきではある。

この例の場合には、最初のプログラムのような、適切なループ構造で表現するべきものをコピペでだらだら書いたこと、およびそのような書き方のために全体を制御するループ構造も適切なものでなかったことによって、修正しにくいバグが入りこんでいた。このバグのために、この関数は普通に使ったのでは正しく動作しなくなっており、このプログラムの作者は上手く動く引数と内部パラメータの組合せを見つけて動かす、というようなことをしていた。

このようなやり方をしていたのでは、無限に時間をかけてもシステム全体がまともに動くようにならない。

この例の場合には、「楽に書く」というのは、実際に正しく動作するプログラムにするために本質的に必要なことである。しかし、コピペでだらだら書いて、そのうちに収拾がつかなくなる、というやり方をするのは特に珍しいことではない(ので、今これを読んでいる人がそういうやり方をしていたとしても、別に恥ずかしく思うとかそういう必要はない。)。これは、つまり、適切な制御構造を利用することでプログラムを楽に書く、というやり方は、少なくとも結構な割合の人にとっては人から教わったり本で勉強したりしなければ身につかないもので、プログラムを書いていれば自然にできるようになるものではない、ということである。

まあ、論文を書くのだって、いくら人の論文を読んでいても自分で書くと全く違うものを書いてしまうわけで、指導は必ず必要である。プログラムを書く、という時にもそういうのが必要、ということであろう。

Previous ToC Next