[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Wei Mi via Phabricator via cfe-commits]

wmi added a comment.

In https://reviews.llvm.org/D36562#880808, @hfinkel wrote:

> You seem to be only changing the behavior for the "separatable" fields, but I suspect you want to change the behavior for the others too. The bitfield would be decomposed into shards, separated by the naturally-sized-and-aligned fields. Each access only loads its shard. For example, in your test case you have:
>
>   struct S3 {
>     unsigned long f1:14;
>     unsigned long f2:18;
>     unsigned long f3:32;
>   };
>   
>
> and you test that, with this option, loading/storing to a3.f3 only access the specific 4 bytes composing f3. But if you load f1 or f2, we're still loading all 8 bytes, right? I think we should only load/store the lower 4 bytes when we access a3.f1 and/or a3.f2.


This is intentional. if the struct S3 is like following:
struct S3 {

  unsigned long f1:14;
  unsigned long f2:32;
  unsigned long f3:18;

};

and if there is no write of a.f2 between a.f1 and a.f3, the loads of a.f1 and a.f2 can still be shared. It is trying to keep the combining opportunity maximally while reducing the cost of accessing naturally-sized-and-aligned fields

> Otherwise, you can again end up with the narrow-store/wide-load problem for nearby fields under a different set of circumstances.

Good catch. It is possible to have the problem indeed. Considering the big perf impact and triaging difficulty of store-forwarding problem, I have to sacrifice the combining opportunity above and take the suggestion just as you describe.

Thanks,
Wei.



================
Comment at: include/clang/Driver/Options.td:1032
 
+def fsplit_bitfields : Flag<["-"], "fsplit-bitfields">,
+  Group<f_clang_Group>, Flags<[CC1Option]>,
----------------
hfinkel wrote:
> I'm not opposed to -fsplit-bitfields, but I'd prefer if we find something more self-explanatory. It's not really clear what "splitting a bitfield" means. Maybe?
> 
>   -fsplit-bitfield-accesses
>   -fdecomposed-bitfield-accesses
>   -fsharded-bitfield-accesses
>   -ffine-grained-bitfield-accesses
> 
> (I think that I prefer -ffine-grained-bitfield-accesses, although it's the longest)
Ok. 


================
Comment at: include/clang/Driver/Options.td:1034
+  Group<f_clang_Group>, Flags<[CC1Option]>,
+  HelpText<"Enable splitting bitfield with legal width and alignment in LLVM.">;
+def fno_split_bitfields : Flag<["-"], "fno-split-bitfields">,
----------------
hfinkel wrote:
> How about?
> 
>   Use separate access for bitfields with legal widths and alignments.
> 
> I don't think that "in LLVM" is needed here (or we could put "in LLVM" on an awful lot of these options).
Sure.


Repository:
  rL LLVM

https://reviews.llvm.org/D36562