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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:376
+    assert(unsigned(Flag) <= ((1u << LLVM_MI_FLAGS_BITS) - 1) &&
+           "Flag is out of range for Flags field");
     Flags |= (uint32_t)Flag;
----------------
These can use `isUInt<LLVM_MI_FLAGS_BITS>(Flag)` etc. (Though maybe not worth including the header if it's not already used.)


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:137
+    uint8_t AsmPrinterFlags : LLVM_MID_ASMPRINTERFLAGS_BITS;
+  })
+  Detail = {0, 0, 0};
----------------
craig.topper wrote:
> 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?
Yes, for this to work on MSVC we would need all of these fields to have the same type (with different widths). However, MSVC does not produce correct layout even with packed structs. It ends up with a 34 byte struct (rather than 32 or 40), so I can only assume we'll get a terrible unaligned mess if we do that.

So I think we should just accept the size increase on MSVC.


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

https://reviews.llvm.org/D153791