[PATCH] D22281: [CodeGen] Refactor MachineMemOperand's Flags enum.

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a reviewer: MatzeB.
MatzeB added a comment.
This revision is now accepted and ready to land.

Looks good to me.

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


================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:92-97
@@ -96,1 +91,8 @@
 public:
+  // 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;
+
----------------
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,
  ...
};
```

================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:100
@@ -97,2 +99,3 @@
   /// Flags values. These may be or'd together.
-  enum MemOperandFlags {
+  enum Flags : uint16_t {
+    // No flags set.
----------------
`MOMaxBits == 8` so this can be `uint8_t` I presume.

================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:102-110
@@ -99,9 +101,11 @@
+    // 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.
----------------
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,
 //...
```


https://reviews.llvm.org/D22281