[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

93–94

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

93–94

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

93–94

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

93–94

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

93–94

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

97

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/ARMBaseInstrInfo.h

bool isUncondBranchOpcode(int Opc) {
   return Opc == ARM::B || Opc == ARM::tB || Opc == ARM::t2B;
 }
 
 // This table shows the VPT instruction variants, i.e. the different
 // mask field encodings, see also B5.6. Predication/conditional execution in
 // the ArmARM.
 
 
-inline static unsigned getARMVPTBlockMask(unsigned NumInsts) {
+inline static ARM::PredBlockMask getARMVPTBlockMask(unsigned NumInsts) {
   switch (NumInsts) {
   case 1:
-    return ARMVCC::T;
+    return ARM::PredBlockMask::T;
   case 2:
-    return ARMVCC::TT;
+    return ARM::PredBlockMask::TT;
   case 3:
-    return ARMVCC::TTT;
+    return ARM::PredBlockMask::TTT;
   case 4:
-    return ARMVCC::TTTT;
+    return ARM::PredBlockMask::TTTT;
   default:
     break;
   };
   llvm_unreachable("Unexpected number of instruction in a VPT block");
 }
 
 
 static inline bool isVPTOpcode(int Opc) {

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp

void ARMLowOverheadLoops::ConvertVPTBlocks(LowOverheadLoop &LoLoop) {
 
   for (auto &Block : LoLoop.getVPTBlocks()) {
     SmallVectorImpl<PredicatedMI> &Insts = Block.getInsts();
     if (Block.HasNonUniformPredicate()) {
       PredicatedMI *Divergent = Block.getDivergent();
       if (isVCTP(Divergent->MI)) {
         // The vctp will be removed, so the size of the vpt block needs to be
         // modified.
-        uint64_t Size = getARMVPTBlockMask(Block.size() - 1);
+        uint64_t Size = (uint64_t)getARMVPTBlockMask(Block.size() - 1);
         Block.getVPST()->getOperand(0).setImm(Size);
         LLVM_DEBUG(dbgs() << "ARM Loops: Modified VPT block mask.\n");
       } else if (Block.IsOnlyPredicatedOn(LoLoop.VCTP)) {
         // The VPT block has a non-uniform predicate but it's entry is guarded
         // only by a vctp, which means we:
         // - Need to remove the original vpst.
         // - Then need to unpredicate any following instructions, until
         //   we come across the divergent vpr def.
if (Block.HasNonUniformPredicate()) {
         while (I != E) {
           InsertAt = &*I;
           ++Size;
           ++I;
         }
         MachineInstrBuilder MIB = BuildMI(*InsertAt->getParent(), InsertAt,
                                           InsertAt->getDebugLoc(),
                                           TII->get(ARM::MVE_VPST));
-        MIB.addImm(getARMVPTBlockMask(Size));
+        MIB.addImm((uint64_t)getARMVPTBlockMask(Size));
         LLVM_DEBUG(dbgs() << "ARM Loops: Removing VPST: " << *Block.getVPST());
         LLVM_DEBUG(dbgs() << "ARM Loops: Created VPST: " << *MIB);
         LoLoop.ToRemove.insert(Block.getVPST());
       }
     } else if (Block.IsOnlyPredicatedOn(LoLoop.VCTP)) {
       // A vpt block which is only predicated upon vctp and has no internal vpr
       // defs:
       // - Remove vpst.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

bool ARMAsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
   // MCOperand, this is stored in a format independent of the
   // condition code: the lowest set bit indicates the end of the
   // encoding, and above that, a 1 bit indicates 'else', and an 0
   // indicates 'then'. E.g.
   //    IT    -> 1000
   //    ITx   -> x100    (ITT -> 0100, ITE -> 1100)
   //    ITxy  -> xy10    (e.g. ITET -> 1010)
   //    ITxyz -> xyz1    (e.g. ITEET -> 1101)
+  // Note: See the ARM::PredBlockMask enum in
+  //   /lib/Target/ARM/Utils/ARMBaseInfo.h
   if (Mnemonic == "it" || Mnemonic.startswith("vpt") ||
       Mnemonic.startswith("vpst")) {
     SMLoc Loc = Mnemonic == "it"  ? SMLoc::getFromPointer(NameLoc.getPointer() + 2) :
                 Mnemonic == "vpt" ? SMLoc::getFromPointer(NameLoc.getPointer() + 3) :
                                     SMLoc::getFromPointer(NameLoc.getPointer() + 4);
     if (ITMask.size() > 3) {
       if (Mnemonic == "it")
         return Error(Loc, "too many conditions on IT instruction");

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

 }
 } // end namespace ARMCC
 
 namespace ARMVCC {
   enum VPTCodes {
     None = 0,
     Then,
     Else
   };
+} // namespace ARMVCC
 
-  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
+namespace ARM {
+  /// Mask values for IT and 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 class PredBlockMask {
+    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 ARM
 
 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");