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

[Guillaume Chatelet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1098
 class CallBase : public Instruction {
+  using CallingConvField = Bitfield<CallingConv::ID, 2, 10>; // Next bit:12
+
----------------
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`


================
Comment at: llvm/include/llvm/IR/Instructions.h:427
 class FenceInst : public Instruction {
+  using OrderingField = Bitfield<AtomicOrdering, 1, 3>; // Next bit:4
+
----------------
The first bit is unused, this leads to an additional shift operation for both get and set.
Also the field was not bounded, making it impossible to know which bits are available.


================
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
----------------
The bit in position `1` is unused?


================
Comment at: llvm/include/llvm/IR/Instructions.h:564
            "CmpXchg instructions can only be atomic.");
-    setInstructionSubclassData((getSubclassDataFromInstruction() & ~0x1c) |
-                               ((unsigned)Ordering << 2));
----------------
The masking pattern here is dubious.


================
Comment at: llvm/include/llvm/IR/Instructions.h:577
            "CmpXchg instructions can only be atomic.");
-    setInstructionSubclassData((getSubclassDataFromInstruction() & ~0xe0) |
-                               ((unsigned)Ordering << 5));
----------------
The masking pattern here is dubious.


================
Comment at: llvm/include/llvm/IR/Instructions.h:720
+  using VolatileField = Bitfield<bool, 0, 1>;
+  using AtomicOrderingField = Bitfield<AtomicOrdering, 2, 3>;
+  using OperationField = Bitfield<BinOp, 5, 4>;
----------------
Same here the bit in position `1` is unused.


================
Comment at: llvm/include/llvm/IR/Instructions.h:743
-    setInstructionSubclassData((SubclassData & 31) |
-                               (Operation << 5));
   }
----------------
The `Operation` field is not bounded making it impossible to know which next bit is usable.


================
Comment at: llvm/lib/IR/Instructions.cpp:1401
-  setInstructionSubclassData((getSubclassDataFromInstruction() & ~(31 << 1)) |
-                             (encode(Align) << 1));
-  assert(getAlign() == Align && "Alignment representation error!");
----------------
Here only the lower bits are masked so theoretically `encode(Align)` could occupy more than 6 bits and start overrun the next field storing `AtomicOrdering`


================
Comment at: llvm/lib/IR/Instructions.cpp:1480
-  setInstructionSubclassData((getSubclassDataFromInstruction() & ~(31 << 1)) |
-                             (encode(Alignment) << 1));
-  assert(getAlign() == Alignment && "Alignment representation error!");
----------------
ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81662