Re: [dpdk-dev] rte_ether_addr_copy() strange comment

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 6/25/2020 4:45 PM, Morten Brørup wrote:
> The function rte_ether_addr_copy() checks for __INTEL_COMPILER and has a comment about "a strange gcc warning". It says:
> 
> static inline void rte_ether_addr_copy(const struct rte_ether_addr *ea_from,
> 				   struct rte_ether_addr *ea_to)
> {
> #ifdef __INTEL_COMPILER
> 	uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
> 	uint16_t *to_words   = (uint16_t *)(ea_to->addr_bytes);
> 
> 	to_words[0] = from_words[0];
> 	to_words[1] = from_words[1];
> 	to_words[2] = from_words[2];
> #else
> 	/*
> 	 * Use the common way, because of a strange gcc warning.
> 	 */
> 	*ea_to = *ea_from;
> #endif
> }
> 
> I can see that from_words discards the const qualifier. Is that the "strange" gcc warning the comment is referring to?
> 
> This goes back to before the first public release of DPDK in 2013, ref. https://git.dpdk.org/dpdk/log/lib/librte_ether/rte_ether.h
> 
> 
> It should be fixed as follows:
> 
> -	uint16_t *from_words = (uint16_t *)(ea_from->addr_bytes);
> -	uint16_t *to_words   = (uint16_t *)(ea_to->addr_bytes);
> +	const uint16_t *from_words = (const uint16_t *)ea_from;
> +	uint16_t       *to_words   = (uint16_t *)ea_to;
> 
> And the consequential question: Is copying the three shorts faster than copying the struct? In other words: Should we get rid of the #ifdef and use the first method only?


I tried to investigate this in godbolt: https://godbolt.org/z/YSmvDn

First I don't see the "strange" gcc warning with various gcc versions there.

Related to the struct copy vs word copy, struct copy seems with less instruction
[1],[2],
my vote to remove ifdef and keep struct copy.


[1] copy as individual function
[1a] gcc 10.1, struct copy:
copy:
        movdqa  (%rsi), %xmm0
        movaps  %xmm0, (%rdi)
        ret

[1b] gcc 10.1, word copy:
copy:
        movzwl  (%rsi), %eax
        movw    %ax, (%rdi)
        movzwl  2(%rsi), %eax
        movw    %ax, 2(%rdi)
        movzwl  4(%rsi), %eax
        movw    %ax, 4(%rdi)
        ret

[1c] icc 19.0.1, struct copy
copy:
        movups    (%rsi), %xmm0                                 #19.13
        movups    %xmm0, (%rdi)                                 #19.13
        ret


[2] gcc 10.1, copy as inline function that knows the data, both seems similar
// .addr = {1, 1, 1, 1, 1, 1},
[2a] struct copy:
...
        movl    $257, %eax
        movw    %ax, 4(%rsp)
        leaq    16(%rsp), %rdi
        movl    $16843009, (%rsp)
        movdqa  (%rsp), %xmm0
        movaps  %xmm0, 16(%rsp)
...

[2b] word copy:
        movl    $257, %eax
        movq    %rsp, %rdi
        movw    %ax, 4(%rsp)
        movl    $16843009, (%rsp)

> 
> PS: I will provide a patch which improves rte_is_broadcast_ether_addr() too. The magic formula here is: return (w[0] & w[1] & w[2]) == 0xFFFF;
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup
> 
>