[libcxx-commits] [PATCH] D121009: [libc++] Simplify how __format_spec::_Flags is packed on AIX

[Hubert Tong via Phabricator via libcxx-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:49
+# define _LIBCPP_PACK_FLAGS_ON_AIX
+#endif
+
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Hm, counter-argument: unless we're going to `#undef` this at the end of the header, it feels like this //is// something generic that might get copy-pasted around, and would have to move into a shared header at that point to avoid multiple definitions. If we're going to localize it into this one file, personally I'd prefer just
> > ```
> > #ifdef _AIX
> > class _LIBCPP_TYPE_VIS __attribute__((__packed__)) _Flags {
> > #else
> > class _LIBCPP_TYPE_VIS _Flags {
> > #endif // _AIX
> > ```
> > (But I don't care much.)
> Agreed, I like yours better.
> 
> @hubert.reinterpretcast I think it would be necessary to know whether this will have to be repeated in many places. I actually don't quite understand what makes this struct so special -- why the compiler packs it on all platforms except AIX. CC @Mordante 
@ldionne, any struct having bitfields on AIX would have at least size 4 unless if marked as packed.

This pattern has shown up twice in the LLVM source:
```
./include/llvm/CodeGen/SelectionDAGNodes.h-464-
./include/llvm/CodeGen/SelectionDAGNodes.h-465-#if defined(_AIX) && (!defined(__GNUC__) || defined(__clang__))
./include/llvm/CodeGen/SelectionDAGNodes.h-466-// Except for GCC; by default, AIX compilers store bit-fields in 4-byte words
./include/llvm/CodeGen/SelectionDAGNodes.h-467-// and give the `pack` pragma push semantics.
./include/llvm/CodeGen/SelectionDAGNodes.h:468:#define BEGIN_TWO_BYTE_PACK() _Pragma("pack(2)")
./include/llvm/CodeGen/SelectionDAGNodes.h:469:#define END_TWO_BYTE_PACK() _Pragma("pack(pop)")
./include/llvm/CodeGen/SelectionDAGNodes.h-470-#else
./include/llvm/CodeGen/SelectionDAGNodes.h-471-#define BEGIN_TWO_BYTE_PACK()
./include/llvm/CodeGen/SelectionDAGNodes.h-472-#define END_TWO_BYTE_PACK()
./include/llvm/CodeGen/SelectionDAGNodes.h-473-#endif
--
./include/llvm/IR/BasicBlock.h-515-private:
./include/llvm/IR/BasicBlock.h-516-#if defined(_AIX) && (!defined(__GNUC__) || defined(__clang__))
./include/llvm/IR/BasicBlock.h-517-// Except for GCC; by default, AIX compilers store bit-fields in 4-byte words
./include/llvm/IR/BasicBlock.h-518-// and give the `pack` pragma push semantics.
./include/llvm/IR/BasicBlock.h:519:#define BEGIN_TWO_BYTE_PACK() _Pragma("pack(2)")
./include/llvm/IR/BasicBlock.h:520:#define END_TWO_BYTE_PACK() _Pragma("pack(pop)")
./include/llvm/IR/BasicBlock.h-521-#else
./include/llvm/IR/BasicBlock.h-522-#define BEGIN_TWO_BYTE_PACK()
./include/llvm/IR/BasicBlock.h-523-#define END_TWO_BYTE_PACK()
./include/llvm/IR/BasicBlock.h-524-#endif
```

The pragma form offers better semantics than the attribute: The attribute form will reduce alignment of other members beyond what is necessary. The pragma form only reduces to the argument value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121009