[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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

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.

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



================
Comment at: include/clang/Driver/Options.td:1032
 
+def fsplit_bitfields : Flag<["-"], "fsplit-bitfields">,
+  Group<f_clang_Group>, Flags<[CC1Option]>,
----------------
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)


================
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">,
----------------
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).


================
Comment at: lib/CodeGen/CGExpr.cpp:1679
+/// field has legal integer width, and its bitfield offset is naturally
+/// aligned, it is better to access the bitfield like a separate integer var.
+static bool IsSeparatableBitfield(const CGBitFieldInfo &Info,
----------------
var -> variable


Repository:
  rL LLVM

https://reviews.llvm.org/D36562