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

[Justin Lebar via llvm-commits]

jlebar added a comment.

Thank you for the review, Matthias and Chandler.


================
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;
+
----------------
MatzeB wrote:
> 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,
>   ...
> };
> ```
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.  :)

================
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.
----------------
MatzeB wrote:
> `MOMaxBits == 8` so this can be `uint8_t` I presume.
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.  :)


https://reviews.llvm.org/D22281