This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros.
AbandonedPublic

Authored by jlebar on Aug 1 2016, 2:32 PM.

Details

Reviewers
None
Summary

The latter lets us assign to a bitfield while also checking that the RHS
fits into the field.

There are many places where we check that the RHS "fits" before
assigning it to a bitfield; this lets us replace those with a single
line (which also correctly infers the size of the LHS).

Diff Detail

Event Timeline

jlebar updated this revision to Diff 66381.Aug 1 2016, 2:32 PM
jlebar retitled this revision from to [Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros..
jlebar updated this object.
jlebar added a reviewer: chandlerc.
jlebar added subscribers: llvm-commits, majnemer, timshen.
chandlerc edited edge metadata.Aug 1 2016, 4:18 PM

You don't have any negative tests. Want to write some death tests behind NDEBUG?

llvm/include/llvm/Support/Bitfields.h
29

You're also relying on a bunch of other things here... Mostly that Type can be memset to all 1s without violating any of its invariants such as that this will create no trap representations.

I'm not sure how to word a static assert such that it catches this, but at the least you should check that it is standard layout and trivially copyable.

35–40

I don't think this will read effectively aftor auto-brief. I think you need the first sentence to be a prose description.

45

s/S/s/

llvm/unittests/Support/BitfieldsTest.cpp
23–24

That's odd, even with parentheses around the macro?

jlebar updated this revision to Diff 66401.Aug 1 2016, 4:41 PM
jlebar marked 4 inline comments as done.
jlebar edited edge metadata.

In IRL conversation, we realized that this patch would be a lot simpler if we
relied on the fact that assigning a too-large value to an unsigned bitfield
field is guaranteed to truncate.

This gets around the assumptions I was making in the LLVM_BITFIELD_WIDTH macro,
that the target type is default-constructible and so on. It's also just less
code.

jlebar added a comment.Aug 1 2016, 4:42 PM

Added death tests. Didn't realize we had those in LLVM.

llvm/unittests/Support/BitfieldsTest.cpp
23–24

EXPECT_EQ uses its args in unevaluated contexts, and that means we end up calling a lambda in an unevaluated context. :)

rnk added a subscriber: rnk.Aug 1 2016, 5:13 PM

Do we really need this macro? I think we should just encourage this pattern:

class Foo {
private:
  unsigned Data : 12;
  ...
public:
  MyEnum getData() { return MyEnum(Data); }
  void setData(MyEnum V) { Data = V; assert(Data == V && "bitfield too small"); }
  ...
};

Sure, it's not compile time checked, but I hope we have tests exercising the use of all enums in our data structures.

In D23035#502855, @rnk wrote:

Do we really need this macro? I think we should just encourage this pattern:

class Foo {
private:
  unsigned Data : 12;
  ...
public:
  MyEnum getData() { return MyEnum(Data); }
  void setData(MyEnum V) { Data = V; assert(Data == V && "bitfield too small"); }
  ...
};

Sure, it's not compile time checked, but I hope we have tests exercising the use of all enums in our data structures.

I don't feel strongly about whether we should package this up in a macro and use the macro to implement such setData methods or not.

If folks want the macro, I'm much happier with the simpler incarnation.

jlebar added a comment.Aug 1 2016, 5:22 PM

I think we should just encourage this pattern

Do you think it would be an improvement to rewrite clang's Expr.h to this pattern? Or, more to my point, D23036? It seems kind of wordy to me, but I'm open to thoughts.

I don't like that this macro obfuscates the code, but I do like that we don't have to rely on people writing correct check-in-range code. Like, it wasn't at all obvious to me that foo.x = y; assert(foo.x == y); was correct, or that it *wouldn't* be correct if the bitfield were signed. The macro gets that stuff right, which I like.

rnk added a comment.Aug 1 2016, 5:36 PM

I think we should just encourage this pattern

Do you think it would be an improvement to rewrite clang's Expr.h to this pattern? Or, more to my point, D23036? It seems kind of wordy to me, but I'm open to thoughts.

You only end up using LLVM_CHECKED_BITFIELD_ASSIGN four times in that patch, so I'm not sure it's worth factoring the assertion out into a macro is worth it.

I don't like that this macro obfuscates the code, but I do like that we don't have to rely on people writing correct check-in-range code. Like, it wasn't at all obvious to me that foo.x = y; assert(foo.x == y); was correct, or that it *wouldn't* be correct if the bitfield were signed. The macro gets that stuff right, which I like.

Either way we have an education problem: either remember this library thing, or remember that round-tripping through the bitfield is the least fragile way to range check it. It's a *little* verbose, but there's enough less magic that I like writing it out a bit more.

jlebar added a comment.Aug 1 2016, 8:18 PM

You only end up using LLVM_CHECKED_BITFIELD_ASSIGN four times in that patch, so I'm not sure it's worth factoring the assertion out into a macro is worth it.

And we only declare three non-single-bit bitfields -- that's a 1.3 calls per field! :)

Joking aside, a corollary of your point of strikes me as an argument against wrapping the bitfield's fields: Most bitfields have size 1 [1]. This idiom would force us to wrap all of them in getters/setters, or otherwise we'd have an inconsistent way of accessing the fields, even though we probably don't care about wrapping the size-one fields. With the macro, all reads work the same for all widths with the macro without requiring you to write five lines of boilerplate per field (when, like you point out, you only really want it for a few of the fields).

Similarly, if you have a bitfield struct with all size-one fields and then want to add a size-two field, the idiom would force you to rewrite all users.

Either way we have an education problem: either remember this library thing, or remember that round-tripping through the bitfield is the least fragile way to range check it.

I guess...remembering that a library exists isn't quite the same as remembering how to do it (and convincing your reviewers that you're right, and so on), though.

I don't want this to come across as though I'm totally sold on the macro. An option would be simply to get rid of the range checks in SelectionDAG, under the assumption that, if you add a value to one of these enums and then use it in SelectionDAG, hopefully you have a test that fails.

[1] Source: Perusing git grep ' : [0-9]\+\>' | grep -v '\?' | grep -v test

jlebar added a comment.Aug 1 2016, 8:20 PM

Actually, better regexp: git grep ' : [0-9]\+;$' | grep -v '\?' | grep -v test

jlebar added a comment.EditedAug 1 2016, 8:27 PM

ed, hit enter

An option would be simply to get rid of the range checks in SelectionDAG, under the assumption that, if you add a value to one of these enums and then use it in SelectionDAG, hopefully you have a test that fails.

In fact, we could even static_assert that e.g. ISD::LAST_LOADEXT_TYPE is <= 2**field_width.

One problem with this is we'd need to define a max field on AtomicOrdering. There's no great way to do that, because either the max == the last value, in which case it's inconsistent with basically every other enum max field, or otherwise the max == last_value + 1, in which case we morally lose strict typing on the enum class.

Still, I kind of like this solution, at least as far as storing enums, because it's checked at compile-time.

jlebar abandoned this revision.Aug 2 2016, 2:26 PM

OK, let's abandon this. I updated D23036 to the idiom as described here.

Thanks, everyone.

jlebar abandoned this revision.Sep 14 2016, 10:15 AM