[CodeGen] Refactor MachineMemOperand's Flags enum.

[jlebar]

Summary

• Give it a shorter name (because we're going to refer to it often from SelectionDAG and friends).

• Specialize FlagsEnumTraits for it, so we can do bitwise ops on it without losing type information.

• Make some enum values constants in MachineMemOperand instead. MOMaxBits should not be a valid Flag.

• Simplify some of the bitwise ops for dealing with Flags.

[jlebar]

Fix typo.

[MatzeB]

How about something like:

uint16_t Flags : 8;
uint16_t Align : 8;

so we can leave the bit fiddeling to the compiler?

[MatzeB]

In D22281#482457, @MatzeB wrote:

How about something like:

uint16_t Flags : 8;
uint16_t Align : 8;

so we can leave the bit fiddeling to the compiler?

Typo, I meant

uint16_t Flags;
uint16_t Align;
```.

[jlebar]

Split flags and alignment into separate variables.

[jlebar]

In D22281#482458, @MatzeB wrote:

so we can leave the bit fiddeling to the compiler?

Well that's a crazy idea. Done, thank you.

[MatzeB]

include/llvm/CodeGen/MachineMemOperand.h

102

You could change this to enum Flags : uint16_t {...} so that spltting the alignment won't make the MemOperands larger than before.
I assume the MOMaxFlag can is unnecessary as well now?

[chandlerc]

include/llvm/CodeGen/MachineMemOperand.h

102

a fixed underlying type can't *shrink* an enum... if it only requires <= 16 bits to represent the largest enumerator, that's how many bits you'll get?

[MatzeB]

include/llvm/CodeGen/MachineMemOperand.h

102

Unfortunately all popular compilers out there choose unsigned/int even for smaller enums, so explicitely shrinking it is important!

[jlebar]

Give the Flags enum an explicit storage type.

include/llvm/CodeGen/MachineMemOperand.h

102

Yes, of course -- this got lost while I was bisecting a bug in my changes. Sorry about that.

[jlebar]

Update to latest version of bitmask enum patch.

[chandlerc]

FYI, this LGTM. Good to get Matthias's Ok before you land it though as this is a more involved change.

[MatzeB]

Looks good to me.

Possible style suggestions below, they are personal style, I won't mind if you decide against changing it:

include/llvm/CodeGen/MachineMemOperand.h

92–97

Outside of the enum I find it more dangerous that those are forgotten when someone updates the enum.

I also wanted to point out that we can solve target flags in a slightly more elegant way. Though we should change one thing at a time and better leave this bit for another day/patch:

enum Flags { ...
  MOInvariant = 16,
  MOFirstTargetFlag = 32,
  ...

in target code:

enum TargetFlags { ...
  MOTargetFlag1 = MOFirstTargetFlag << 0,
  MOTargetFlag2 = MOFirstTargetFlag << 1,
  MOTargetFlag3 = MOFirstTargetFlag << 2,
  ...
};

92–104

MOMaxBits == 8 so this can be uint8_t I presume.

94–102

Another bit possibly for another day/another patch. I prefer this style:

MONone = 0,
MOLoad = 1u << 0,
MOStore = 1u << 1,
MOVolatile = 1u << 2,
MONonTemporal = 1u << 3,
MOInvariant = 1u << 4,
//...

[jlebar]

Thank you for the review, Matthias and Chandler.

include/llvm/CodeGen/MachineMemOperand.h

92–97

While I agree with your point about the increased likelihood that these constants will fall out of sync if they're outside the enum, I nonetheless think it's right for them to be there, because otherwise you could do

Flags f = MOInvariant | MOTargetStartBit;

which is a (logical) type error.

Your idea about naming explicit target flags in the enum is interesting. I'm afraid it may be obfuscating, but maybe you can do

constexpr Flags MOMyNamedTargetFlag = Flags::MOTargetFlag2;

I guess that's better than

constexpr Flags MOMyNamedTargetFlag = Flags::MOFirstTargetFlag << 1;

although the target flags are used so infrequently that it doesn't bother me too much. There are much bigger fish to fry here. :)

92–104

I think I'd rather keep it as a uint16 -- we have the space in the structure, and I would rather make someone think twice before using that space for something other than flags. (Also the reason I'm doing all this refactoring is I want to add another bit here. :)

[jlebar]

Possible style suggestions below, they are personal style, I won't mind if you decide against changing it:

You were right, sending a patch with all these changes. Better than forgetting to define the static const members in the cpp file...

include/llvm/CodeGen/MachineMemOperand.h

 // description of a memory reference. It is used to help track dependencies
 // in the backend.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_MACHINEMEMOPERAND_H
 #define LLVM_CODEGEN_MACHINEMEMOPERAND_H
 
+#include "llvm/ADT/FlagsEnum.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/CodeGen/PseudoSourceValue.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Value.h"  // PointerLikeTypeTraits<Value*>
 #include "llvm/Support/DataTypes.h"
 
 namespace llvm {
 

 /// A description of a memory reference used in the backend.
 /// Instead of holding a StoreInst or LoadInst, this class holds the address
 /// Value of the reference along with a byte size and offset. This allows it
 /// to describe lowered loads and stores. Also, the special PseudoSourceValue
 /// objects can be used to represent loads and stores to memory locations
 /// that aren't explicit in the regular LLVM IR.
 ///
 class MachineMemOperand {
-  MachinePointerInfo PtrInfo;
-  uint64_t Size;
-  unsigned Flags;
-  AAMDNodes AAInfo;
-  const MDNode *Ranges;
-
 public:
-  /// Flags values. These may be or'd together.
-  enum MemOperandFlags {
+  // This is the number of bits we need to represent flags.
+  static constexpr unsigned MOMaxBits = 8;
+
+  // Target hints allow target passes to annotate memory operations.
+  static constexpr unsigned MOTargetStartBit = 5;
+  static constexpr unsigned MOTargetNumBits = 3;
+
+  /// Flags values. These may be or'd together.  You can perform bitwise
+  /// operations on this enum without static_cast, because we've opted in below
+  /// via ENABLE_FLAGS_ENUM.
+  enum Flags : uint16_t {
+    // No flags set.
+    MONone = 0,
     /// The memory access reads data.
     MOLoad = 1,
     /// The memory access writes data.
     MOStore = 2,
     /// The memory access is volatile.
     MOVolatile = 4,
     /// The memory access is non-temporal.
     MONonTemporal = 8,
     /// The memory access is invariant.
     MOInvariant = 16,
-    // Target hints allow target passes to annotate memory operations.
-    MOTargetStartBit = 5,
-    MOTargetNumBits = 3,
-    // This is the number of bits we need to represent flags.
-    MOMaxBits = 8
+
+    // Maximum MemOperandFlag value (inclusive).  This is needed for
+    // correctness, because the compiler can assume that we don't write to bits
+    // in this enum above the highest-order one that's set by one of the enum's
+    // values.  Must be one less than a power of 2.
+    MOMaxFlag = (1 << MOMaxBits) - 1,
   };
 
+private:
+  MachinePointerInfo PtrInfo;
+  uint64_t Size;
+  Flags FlagVals;
+  uint16_t BaseAlignLog2; // log_2(base_alignment) + 1
+  AAMDNodes AAInfo;
+  const MDNode *Ranges;
+
+public:
   /// Construct a MachineMemOperand object with the specified PtrInfo, flags,
   /// size, and base alignment.
-  MachineMemOperand(MachinePointerInfo PtrInfo, unsigned flags, uint64_t s,
+  MachineMemOperand(MachinePointerInfo PtrInfo, Flags flags, uint64_t s,
                     unsigned base_alignment,
                     const AAMDNodes &AAInfo = AAMDNodes(),
                     const MDNode *Ranges = nullptr);
 
   const MachinePointerInfo &getPointerInfo() const { return PtrInfo; }
 
   /// Return the base address of the memory access. This may either be a normal
   /// LLVM IR Value, or one of the special values used in CodeGen.
   /// Special values are those obtained via
   /// PseudoSourceValue::getFixedStack(int), PseudoSourceValue::getStack, and
   /// other PseudoSourceValue member functions which return objects which stand
   /// for frame/stack pointer relative references and other special references
   /// which are not representable in the high-level IR.
   const Value *getValue() const { return PtrInfo.V.dyn_cast<const Value*>(); }
 
   const PseudoSourceValue *getPseudoValue() const {
     return PtrInfo.V.dyn_cast<const PseudoSourceValue*>();
   }
 
   const void *getOpaqueValue() const { return PtrInfo.V.getOpaqueValue(); }
 
-  /// Return the raw flags of the source value, \see MemOperandFlags.
-  unsigned int getFlags() const { return Flags & ((1 << MOMaxBits) - 1); }
+  /// Return the raw flags of the source value, \see Flags.
+  Flags getFlags() const { return FlagVals; }
 
   /// Bitwise OR the current flags with the given flags.
-  void setFlags(unsigned f) { Flags |= (f & ((1 << MOMaxBits) - 1)); }
+  void setFlags(Flags f);
 
   /// For normal values, this is a byte offset added to the base address.
   /// For PseudoSourceValue::FPRel values, this is the FrameIndex number.
   int64_t getOffset() const { return PtrInfo.Offset; }
 
   unsigned getAddrSpace() const { return PtrInfo.getAddrSpace(); }
 
   /// Return the size in bytes of the memory reference.
   uint64_t getSize() const { return Size; }
 
   /// Return the minimum known alignment in bytes of the actual memory
   /// reference.
   uint64_t getAlignment() const;
 
   /// Return the minimum known alignment in bytes of the base address, without
   /// the offset.
-  uint64_t getBaseAlignment() const { return (1u << (Flags >> MOMaxBits)) >> 1; }
+  uint64_t getBaseAlignment() const { return (1u << BaseAlignLog2) >> 1; }
 
   /// Return the AA tags for the memory reference.
   AAMDNodes getAAInfo() const { return AAInfo; }
 
   /// Return the range tag for the memory reference.
   const MDNode *getRanges() const { return Ranges; }
 
-  bool isLoad() const { return Flags & MOLoad; }
-  bool isStore() const { return Flags & MOStore; }
-  bool isVolatile() const { return Flags & MOVolatile; }
-  bool isNonTemporal() const { return Flags & MONonTemporal; }
-  bool isInvariant() const { return Flags & MOInvariant; }
+  bool isLoad() const { return FlagVals & MOLoad; }
+  bool isStore() const { return FlagVals & MOStore; }
+  bool isVolatile() const { return FlagVals & MOVolatile; }
+  bool isNonTemporal() const { return FlagVals & MONonTemporal; }
+  bool isInvariant() const { return FlagVals & MOInvariant; }
 
   /// Returns true if this memory operation doesn't have any ordering
   /// constraints other than normal aliasing. Volatile and atomic memory
   /// operations can't be reordered.
   ///
   /// Currently, we don't model the difference between volatile and atomic
   /// operations. They should retain their ordering relative to all memory
   /// operations.
public:
   }
 
   friend bool operator!=(const MachineMemOperand &LHS,
                          const MachineMemOperand &RHS) {
     return !(LHS == RHS);
   }
 };
 
+ENABLE_FLAGS_ENUM(MachineMemOperand::Flags, MachineMemOperand::MOMaxFlag);
+
+// Needs to appear after the ENABLE_FLAGS_ENUM.
+inline void MachineMemOperand::setFlags(Flags f) { FlagVals |= f; }
+
 inline raw_ostream &operator<<(raw_ostream &OS, const MachineMemOperand &MRO) {
   MRO.print(OS);
   return OS;
 }
 
 } // End llvm namespace
 
 #endif

lib/CodeGen/MachineFunction.cpp

MachineFunction::DeleteMachineBasicBlock(MachineBasicBlock *MBB) {
   BasicBlockRecycler.Deallocate(Allocator, MBB);
 }
 
 MachineMemOperand *
 MachineFunction::getMachineMemOperand(MachinePointerInfo PtrInfo, unsigned f,
                                       uint64_t s, unsigned base_alignment,
                                       const AAMDNodes &AAInfo,
                                       const MDNode *Ranges) {
-  return new (Allocator) MachineMemOperand(PtrInfo, f, s, base_alignment,
-                                           AAInfo, Ranges);
+  // FIXME: Get rid of this static_cast and make getMachineOperand take a
+  // MachineMemOperand::Flags param.
+  return new (Allocator)
+      MachineMemOperand(PtrInfo, static_cast<MachineMemOperand::Flags>(f), s,
+                        base_alignment, AAInfo, Ranges);
 }
 
 MachineMemOperand *
 MachineFunction::getMachineMemOperand(const MachineMemOperand *MMO,
                                       int64_t Offset, uint64_t Size) {
   if (MMO->getValue())
     return new (Allocator)
                MachineMemOperand(MachinePointerInfo(MMO->getValue(),

lib/CodeGen/MachineInstr.cpp

MachinePointerInfo MachinePointerInfo::getGOT(MachineFunction &MF) {
   return MachinePointerInfo(MF.getPSVManager().getGOT());
 }
 
 MachinePointerInfo MachinePointerInfo::getStack(MachineFunction &MF,
                                                 int64_t Offset) {
   return MachinePointerInfo(MF.getPSVManager().getStack(), Offset);
 }
 
-MachineMemOperand::MachineMemOperand(MachinePointerInfo ptrinfo, unsigned f,
+MachineMemOperand::MachineMemOperand(MachinePointerInfo ptrinfo, Flags f,
                                      uint64_t s, unsigned int a,
                                      const AAMDNodes &AAInfo,
                                      const MDNode *Ranges)
-  : PtrInfo(ptrinfo), Size(s),
-    Flags((f & ((1 << MOMaxBits) - 1)) | ((Log2_32(a) + 1) << MOMaxBits)),
+    : PtrInfo(ptrinfo), Size(s), FlagVals(f), BaseAlignLog2(Log2_32(a) + 1),
       AAInfo(AAInfo), Ranges(Ranges) {
+  assert(MOMaxFlag == (1 << MOMaxBits) - 1 &&
+         "MOMaxFlag and MOMaxBits have fallen out of sync.");
   assert((PtrInfo.V.isNull() || PtrInfo.V.is<const PseudoSourceValue*>() ||
           isa<PointerType>(PtrInfo.V.get<const Value*>()->getType())) &&
          "invalid pointer value");
   assert(getBaseAlignment() == a && "Alignment is not a power of 2!");
   assert((isLoad() || isStore()) && "Not a load/store!");
 }
 
 /// Profile - Gather unique data for the object.
 ///
 void MachineMemOperand::Profile(FoldingSetNodeID &ID) const {
   ID.AddInteger(getOffset());
   ID.AddInteger(Size);
   ID.AddPointer(getOpaqueValue());
-  ID.AddInteger(Flags);
+  ID.AddInteger(getFlags());
+  ID.AddInteger(getBaseAlignment());
 }
 
 void MachineMemOperand::refineAlignment(const MachineMemOperand *MMO) {
   // The Value and Offset may differ due to CSE. But the flags and size
   // should be the same.
   assert(MMO->getFlags() == getFlags() && "Flags mismatch!");
   assert(MMO->getSize() == getSize() && "Size mismatch!");
 
   if (MMO->getBaseAlignment() >= getBaseAlignment()) {
     // Update the alignment value.
-    Flags = (Flags & ((1 << MOMaxBits) - 1)) |
-      ((Log2_32(MMO->getBaseAlignment()) + 1) << MOMaxBits);
+    BaseAlignLog2 = Log2_32(MMO->getBaseAlignment()) + 1;
     // Also update the base and offset, because the new alignment may
     // not be applicable with the old ones.
     PtrInfo = MMO->PtrInfo;
   }
 }
 
 /// getAlignment - Return the minimum known alignment in bytes of the
 /// actual memory reference.

lib/Target/AArch64/AArch64InstrInfo.cpp

 /// Set a flag on the first MachineMemOperand to suppress pairing.
 void AArch64InstrInfo::suppressLdStPair(MachineInstr &MI) const {
   if (MI.memoperands_empty())
     return;
 
   static_assert(MOSuppressPair < (1 << MachineMemOperand::MOTargetNumBits),
                 "Too many target MO flags");
   (*MI.memoperands_begin())
-      ->setFlags(MOSuppressPair << MachineMemOperand::MOTargetStartBit);
+      ->setFlags(static_cast<MachineMemOperand::Flags>(
+          MOSuppressPair << MachineMemOperand::MOTargetStartBit));
 }
 
 bool AArch64InstrInfo::isUnscaledLdSt(unsigned Opc) const {
   switch (Opc) {
   default:
     return false;
   case AArch64::STURSi:
   case AArch64::STURDi: