[PATCH] D153791: [MachineInst] Bump NumOperands back up to 24bits

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:137
+    uint8_t AsmPrinterFlags : LLVM_MID_ASMPRINTERFLAGS_BITS;
+  })
+  Detail = {0, 0, 0};
----------------
jroelofs wrote:
> nikic wrote:
> > jroelofs wrote:
> > > nikic wrote:
> > > > Why do we need the packed struct wrapper here? Doesn't defining these bitfields directly in the class work?
> > > The two 24-bit fields need to be part of a packed struct, otherwise we get some padding [1]. We can either do that around the whole struct, or just these 56 bits. I don't have a strong opinion either way.
> > > 
> > > 1: https://clang.godbolt.org/z/4Kxs4fKc7
> > Can you avoid the need for that by reordering the fields to be 24 8 24 8? E.g. Flags, AsmPrinterFlags, NumOperands, CapOperands.
> Good eye! Yeah, that works: https://clang.godbolt.org/z/q1onMv94e
If I remember right, doesn't MSVC pack bit fields differently than clang/gcc?


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

https://reviews.llvm.org/D153791