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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
