[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

John McCall rjmccall at apple.com
Thu Sep 13 00:43:23 PDT 2012


On Sep 13, 2012, at 12:22 AM, Chandler Carruth wrote:
> With all that discussion, here is an updated patch. I've included most of the tricky test cases described here (where they seemed valuable) along with lots of comments to help document some of this discussion and the conclusions.

+  // See if there are other bits in the bitfield's storage we'll need to load
+  // and mask together with source before storing.
+  if (Info.StorageSize != Info.Size) {
+    assert(Info.StorageSize > Info.Size && "Invalid bitfield size.");

Info.StorageSize is always a multiple of the byte size, right?  If a bitfield
is all alone in its storage unit, shouldn't we be able to avoid the
load/mask/or stuff?

+    // Mask out the original value.
+    Val = Builder.CreateAnd(Val,
+                            ~llvm::APInt::getBitsSet(Info.StorageSize,
+                                                     Info.Offset,
+                                                     Info.Offset + Info.Size),
+                            "bf.clear");

This masking is unnecessary if the value is known to be all-ones.
The backend can do this peephole pretty easily, but it's worth doing
ourselves for -O0 goodness.

+    // Or together the unchanged values and the source value.
+    SrcVal = Builder.CreateOr(Val, SrcVal, "bf.set");

And analogously, this is unnecessary if the value is known to be
all-zeroes.

+  /// The storage size in bits which should be used when accessing this
+  /// bitfield.
+  unsigned StorageSize;

This should document any invariants like "this is always a multiple of
the bit-width of a char".

-    assert(!FD->isBitField() && "Invalid call for bit-field decl!");

Is removing this assertion important?  It's a nice way to ensure that
code isn't naively accessing a bitfield like a normal field.

I'll admit to only skimming right now;  if you want a full review,
feel free to wait longer. :)

John.



More information about the cfe-commits mailing list