Re: [PATCH] memset with cache line size fix.

["Steve Munroe" <sjmunroe at us dot ibm dot com>]

> However, now that you mention it, here are some comments.
>
> 1.
>
> +    .section           ".sdata","aw"
>
> is wrong, it fills up the user's .sdata section (when linking
> statically), use .data instead;

OK.

> + /* Establishes GOT addressability so we can load__cache_line_size
> +    from static. This value was set from the aux vector duringstartup.
*/
> +   bl    _GLOBAL_OFFSET_TABLE_@local-4
> +   mflr  rGOT
> +   lwz   rGOT,__cache_line_size@got(rGOT)
> +   lwz   rCLS,0(rGOT)
> +   mtlr  rTMP

> doesn't handle the -fno-pic case, plus it fills the user's .got
> section (when linking statically).

OK. try.

+ #ifdef SHARED
+     mflr  rTMP
+ /*  If the remaining length is less the 32 bytes, don't bother getting
+     the cache line size */
+     beq   L(medium)
+
+ /*  Establishes GOT addressability so we can load __cache_line_size
+     from static. This value was set from the aux vector during startup.
*/
+     bl    _GLOBAL_OFFSET_TABLE_@local-4
+     mflr  rGOT
+     lwz   rGOT,__cache_line_size@got(rGOT)
+     lwz   rCLS,0(rGOT)
+     mtlr  rTMP
+ #else
+     lis   rCLS,__cache_line_size@ha
+     lwz   rCLS,__cache_line_size@l(rCLS)
+ #endif

> 3. Formatting, see Roland's comments on the more recent patches.

It is not clear to me what formatting sins I have committed. I have gone
over the patch again and removed some tabs but I can't find anything wrong.
Please be specific!

> 4. Can you explain
>
> +  dcbt             0,rMEMP
>
> more?  I think it should probably be 'dcbtst'.

It turns out that most PowerPC64 implementation don't implement dcbt or
dcbtst (execute as noops). And for those that do they have the same
behavior. But using dcbtst does little harm for 64-bit and may help on
32-bit systems, so I made this change.

> 5. This chunk is not C code; could use an explanation; and contains a
>    Mysterious Whitespace Change.
>
> ***************
> *** 75,80 ****
>         auxvec = ubp_ev;
>         while (*(char *__unbounded *__unbounded) auxvec != NULL)
> !          ++auxvec;
> !       ++auxvec;
>  #ifndef SHARED
>        _dl_aux_init ((ElfW(auxv_t) *) auxvec);
> --- 97,103 ----
>         auxvec = ubp_ev;
>         while (*(char *__unbounded *__unbounded) auxvec != NULL)
> !                ++(char**)auxvec;
> !       ++(char**)auxvec;
> !
>  #ifndef SHARED
>         _dl_aux_init ((ElfW(auxv_t) *) auxvec);

There is an important difference between "++auxvec" and "++(char**)auxvec".
The first case generates unaligned pointer loads.

   2a584:   2c 00 00 00       cmpwi r0,0
   2a588:   7d 3f 4b 78       mr    r31,r9
   2a58c:   7f e5 fb 78       mr    r5,r31
   2a590:   41 82 00 10       beq   2a5a0 <__libc_start_main+0x90>
   2a594:   84 1f 00 01       lwzu  r0,1(r31)
   2a598:   2c 00 00 00       cmpwi r0,0
   2a59c:   40 82 ff f8       bne   2a594 <__libc_start_main+0x84>
   2a5a0:   3b ff 00 01       addi  r31,r31,1

which I believe is incorrect. The second for generates the correct word
aligned auxvec scan.

   2a584:   2c 00 00 00       cmpwi r0,0
   2a588:   7d 3f 4b 78       mr    r31,r9
   2a58c:   7f e5 fb 78       mr    r5,r31
   2a590:   41 82 00 10       beq   2a5a0 <__libc_start_main+0x90>
   2a594:   84 1f 00 01       lwzu  r0,4(r31)
   2a598:   2c 00 00 00       cmpwi r0,0
   2a59c:   40 82 ff f8       bne   2a594 <__libc_start_main+0x84>
   2a5a0:   3b ff 00 01       addi  r31,r31,4

This is even more important of ppc64 because the offset field of the ldu
instruction is only 14 bits (the low order two bits for the offset are
forced to '00'). So the "++auxvec" expression fails to compile for ppc64
and only the "++(char**)auxvec" expression generates correct code.

(See attached file: memset225a-patch.txt)

Attachment: memset225a-patch.txt
Description: Binary data