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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs 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;
----------------
nikic wrote:
> These can use `isUInt<LLVM_MI_FLAGS_BITS>(Flag)` etc. (Though maybe not worth including the header if it's not already used.)
Thanks for the pointer. ArrayRecycler.h pulls it in, so it's low overhead to use it here.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:137
+    uint8_t AsmPrinterFlags : LLVM_MID_ASMPRINTERFLAGS_BITS;
+  })
+  Detail = {0, 0, 0};
----------------
nikic wrote:
> 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.
Does MSVC have an analog to `-fdump-record-layouts`?


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

https://reviews.llvm.org/D153791