[Target][ARM] Change VPTMaskValues to the correct encoding

[Pierre-vh]

Summary

VPTMaskValue was using the "instruction" encoding to represent the masks (= the same encoding as the one used by the instructions in an object file), but it is only used to build MCOperands, so it should use the MCOperand encoding of the masks, which is slightly different.

Note that this has gone unnoticed for a long time because the encodings are identical for most masks (T, TT, TTT, etc.). We only noticed this (with the help of @simon_tatham
) while we were working on upgrading the MVE VPT Block insertion pass because incorrect assembly was generated for things like TTET.

(Adding @samparker to the list of reviewers as he added this enum I believe)

[simon_tatham]

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

103

Since these mask values are now encoded in the common format used by the MCOperand for both VPT and IT instructions, perhaps they should have a more generic name than VPTMaskValue, and live in the ARM namespace instead of ARMVCC?

[Pierre-vh]

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

103

Sure, this is a good idea, but I don't think that I know enough about the backend to find a sensible name for this enum.
Do you have a suggestion? Perhaps something like BlockMaskValue would work ?

[simon_tatham]

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

103

I agree it's hard to find a good name (both concise and recognizable).

Your suggested name isn't bad, because 'block' and 'mask' are both words used in the ARMARM descriptions of both IT and VPT instructions. I think I'd remove 'Value' (it doesn't add very much description) and replace it with something that hints at conditionalization or predication. How about enum class CondBlockMask, in the llvm::ARM namespace? Or PredBlockMask if you prefer.

Also, since you've copied and pasted my comment from ARMAsmParser.cpp to here, you might add a note at the site of the original comment, mentioning that an enum of the mask values is available here.

[Pierre-vh]

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

103

What do you think about this change?

namespace ARMVCC {
  enum VPTCodes {
    /* etc */
  };
} // namespace ARMVCC

namespace ARM {
  /// Mask values for VPT Blocks, to be used by MCOperands.
  /// Note that this is different from the "real" encoding used by the
  /// instructions. In this encoding, the lowest set bit indicates the end of
  /// the encoding, and above that, "1" indicates an else, while "0" indicates
  /// a then.
  ///   Tx = x100
  ///   Txy = xy10
  ///   Txyz = xyz1
  enum PredBlockMask {
    /* etc */
  };
} // namespace ARM

Also, should PredBlockMask stay in that file, or should I move it to another file? (Where?)

[simon_tatham]

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

103

I think I'd make it an enum class, because otherwise, you're putting some very generic names like T into a namespace that's very widely used. If you see var = PredBlockMask::T; in code, you know where to look for the definition; if you see var = T; then you probably look for a variable in the containing function before thinking to look here.

This seems like a fine file to put it in, though.

[Pierre-vh]

• The Enum is now an Enum-Class
• Added comment in ARMASMParser

Note that this won't compile without the child revision (the update to the MVE VPT Block Insertion Pass) being merged, so I won't commit this until that patch is approved (so I can commit both at the same time)

[simon_tatham]

LGTM, with only one tiny remaining nitpick.

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

107

Perhaps adjust this comment to say "IT and VPT blocks", since the same values are appropriate to either.

[Pierre-vh]

Changed "VPT Blocks" to "IT and VPT Blocks" in ARMBaseInfo.h, line 97

llvm/lib/Target/ARM/Utils/ARMBaseInfo.h

 
 namespace ARMVCC {
   enum VPTCodes {
     None = 0,
     Then,
     Else
   };
 
+  /// Mask values for VPT Blocks, to be used by MCOperands.
+  /// Note that this is different from the "real" encoding used by the
+  /// instructions. In this encoding, the lowest set bit indicates the end of
+  /// the encoding, and above that, "1" indicates an else, while "0" indicates
+  /// a then.
+  ///   Tx = x100
+  ///   Txy = xy10
+  ///   Txyz = xyz1
   enum VPTMaskValue {
-    T     =  8, // 0b1000
-    TT    =  4, // 0b0100
-    TE    = 12, // 0b1100
-    TTT   =  2, // 0b0010
-    TTE   =  6, // 0b0110
-    TEE   = 10, // 0b1010
-    TET   = 14, // 0b1110
-    TTTT  =  1, // 0b0001
-    TTTE  =  3, // 0b0011
-    TTEE  =  5, // 0b0101
-    TTET  =  7, // 0b0111
-    TEEE  =  9, // 0b1001
-    TEET  = 11, // 0b1011
-    TETT  = 13, // 0b1101
-    TETE  = 15  // 0b1111
+    T = 0b1000,
+    TT = 0b0100,
+    TE = 0b1100,
+    TTT = 0b0010,
+    TTE = 0b0110,
+    TEE = 0b1110,
+    TET = 0b1010,
+    TTTT = 0b0001,
+    TTTE = 0b0011,
+    TTEE = 0b0111,
+    TTET = 0b0101,
+    TEEE = 0b1111,
+    TEET = 0b1101,
+    TETT = 0b1001,
+    TETE = 0b1011
   };
-}
+} // namespace ARMVCC
 
 inline static const char *ARMVPTPredToString(ARMVCC::VPTCodes CC) {
   switch (CC) {
   case ARMVCC::None:  return "none";
   case ARMVCC::Then:  return "t";
   case ARMVCC::Else:  return "e";
   }
   llvm_unreachable("Unknown VPT code");