<div dir="ltr">When I spent some time looking at this back in March when Bill mentioned it on IRC. I think I saw a write to one bit in one of the 8-bit pieces and then a read of that bit and a bit from the adjacent byte. So we used a narrow store and then a wider load due to the 2 bits.<div><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">~Craig</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 26, 2020 at 4:32 PM John McCall via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 26 May 2020, at 18:28, Bill Wendling via llvm-dev wrote:<br>
<br>
> We're running into an interesting issue with the Linux kernel, and<br>
> wanted advice on how to proceed.<br>
><br>
> Example of what we're talking about: <a href="https://godbolt.org/z/ABGySq" rel="noreferrer" target="_blank">https://godbolt.org/z/ABGySq</a><br>
><br>
> The issue is that when working with a bitfield a load may happen<br>
> quickly after a store. For instance:<br>
><br>
> struct napi_gro_cb {<br>
>     void *frag;<br>
>     unsigned int frag_len;<br>
>     u16 flush;<br>
>     u16 flush_id;<br>
>     u16 count;<br>
>     u16 gro_remcsum_start;<br>
>     unsigned long age;<br>
>     u16 proto;<br>
>     u8 same_flow : 1;<br>
>     u8 encap_mark : 1;<br>
>     u8 csum_valid : 1;<br>
>     u8 csum_cnt : 3;<br>
>     u8 free : 2;<br>
>     u8 is_ipv6 : 1;<br>
>     u8 is_fou : 1;<br>
>     u8 is_atomic : 1;<br>
>     u8 recursive_counter : 4;<br>
>     __wsum csum;<br>
>     struct sk_buff *last;<br>
> };<br>
><br>
> void dev_gro_receive(struct sk_buff *skb)<br>
> {<br>
> ...<br>
>         same_flow = NAPI_GRO_CB(skb)->same_flow;<br>
> ...<br>
> }<br>
><br>
> Right before the "same_flow = ... ->same_flow;" statement is executed,<br>
> a store is made to the bitfield at the end of a called function:<br>
><br>
>     NAPI_GRO_CB(skb)->same_flow = 1;<br>
><br>
> The store is a byte:<br>
><br>
>     orb    $0x1,0x4a(%rbx)<br>
><br>
> while the read is a word:<br>
><br>
>     movzwl 0x4a(%r12),%r15d<br>
><br>
> The problem is that between the store and the load the value hasn't<br>
> been retired / placed in the cache. One would expect store-to-load<br>
> forwarding to kick in, but on x86 that doesn't happen because x86<br>
> requires the store to be of equal or greater size than the load. So<br>
> instead the load takes the slow path, causing unacceptable slowdowns.<br>
><br>
> GCC gets around this by using the smallest load for a bitfield. It<br>
> seems to use a byte for everything, at least in our examples. From the<br>
> comments, this is intentional, because according to the comments<br>
> (which are never wrong) C++0x doesn't allow one to touch bits outside<br>
> of the bitfield. (I'm not a language lawyer, but take this to mean<br>
> that gcc is trying to minimize which bits it's accessing by using byte<br>
> stores and loads whenever possible.)<br>
><br>
> The question I have is what should we do to fix this issue? Once we<br>
> get to LLVM IR, the information saying that we're accessing a bitfield<br>
> is gone. We have a few options:<br>
><br>
> * We can glean this information from how the loaded value is used and<br>
> fix this during DAG combine, but it seems a bit brittle.<br>
><br>
> * We could correct the size of the load during the front-end's code<br>
> generation. This benefits from using all of LLVM's passes on the code.<br>
><br>
> * We could perform the transformation in another place, possible in <br>
> MIR or MC.<br>
><br>
> What do people think?<br>
<br>
Clang used to generate narrower loads and stores for bit-fields, but a<br>
long time ago it was intentionally changed to generate wider loads<br>
and stores, IIRC by Chandler.  There are some cases where I think the<br>
“new” code goes overboard, but in this case I don’t particularly <br>
have<br>
an issue with the wider loads and stores.  I guess we could make a<br>
best-effort attempt to stick to the storage-unit size when the<br>
bit-fields break evenly on a boundary.  But mostly I think the <br>
frontend’s<br>
responsibility ends with it generating same-size accesses in both<br>
places, and if inconsistent access sizes trigger poor performance,<br>
the backend should be more careful about intentionally changing access<br>
sizes.<br>
<br>
John.<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>