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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 8 inline comments as done.
gchatelet added inline comments.


================
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;
----------------
serge-sans-paille wrote:
> if `sizeof(1ULL) == 64` and `size == 64`` then you're doomed. what about `~static_cast<type>(0) >> (8 * sizeof(type)  - size)` ?
Thx for your comment, I had to be more careful about signed-ness but I believe the new implementation is much better,



================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1098
 class CallBase : public Instruction {
+  using CallingConvField = Bitfield<CallingConv::ID, 2, 10>; // Next bit:12
+
----------------
craig.topper wrote:
> gchatelet wrote:
> > Previous code was not explicit about the size of the packed bit, so it was possible to pass in values that would be truncated when stored (values `>=8192`)
> > Additionally the first two bits were left unused which would force a shift operation for both get and set.
> > 
> > I believe we can just make the bitfield start at `0`
> Isn't TailCallKind from CallInst in bits 0 and 1?
You're right, thx for pointing this out it was not obvious.


================
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");
----------------
serge-sans-paille wrote:
> Looks like this is breaking the abstraction. Make this a (static) method?
The abstraction will now check bounds for enums.


================
Comment at: llvm/include/llvm/IR/Instructions.h:531
+  using VolatileField = Bitfield<bool, 0, 1>;                  // Next bit:1
+  using SuccessOrderingField = Bitfield<AtomicOrdering, 2, 3>; // Next bit:5
+  using FailureOrderingField = Bitfield<AtomicOrdering, 5, 3>; // Next bit:8
----------------
craig.topper wrote:
> gchatelet wrote:
> > The bit in position `1` is unused?
> It used to be syncscope before that was extended to support arbitrary scopes instead of just single thread and system
Thank you, I've updated the patch to reflect this knowledge.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81662