[PATCH] D81662: [NFC] Use ADT/Bitfields in Instructions

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

I really like the extra abstraction layer! Some reviews inline.



================
Comment at: llvm/include/llvm/ADT/Bitfields.h:75
+  static_assert(size > 0, "Size must be non zero");
+  static_assert(lastBit <= 64, "Data must fit in 64 bits");
+  static constexpr uint64_t max_value = (1ULL << size) - 1ULL;
----------------
Shouldn't this be `(8 * sizeof(type))` instead of 64?


================
Comment at: llvm/include/llvm/ADT/Bitfields.h:76
+  static_assert(lastBit <= 64, "Data must fit in 64 bits");
+  static constexpr uint64_t max_value = (1ULL << size) - 1ULL;
+  static constexpr uint64_t mask = max_value << offset;
----------------
if `sizeof(1ULL) == 64` and `size == 64`` then you're doomed. what about `~static_cast<type>(0) >> (8 * sizeof(type)  - size)` ?


================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1357
   void setCallingConv(CallingConv::ID CC) {
-    auto ID = static_cast<unsigned>(CC);
-    assert(!(ID & ~CallingConv::MaxID) && "Unsupported calling convention");
-    setInstructionSubclassData((getSubclassDataFromInstruction() & 3) |
-                               (ID << 2));
+    assert(!(unsigned(CC) & ~CallingConv::MaxID) &&
+           "Unsupported calling convention");
----------------
Looks like this is breaking the abstraction. Make this a (static) method?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81662/new/

https://reviews.llvm.org/D81662