[SveEmitter] Fix encoding/decoding of SVETypeFlags

[sdesmalen]

Summary

This issue was introduced when reworking D75861. The bug isn't
actually hit with current unit tests because the contiguous loads/stores
infer the EltType and the MemEltType from the pointer and result, rather
than using the flags. But it will be needed for other intrinsics, such as
gather/scatter.

[andwar]

Cheers for the fix @sdesmalen !

clang/include/clang/Basic/TargetBuiltins.h

187

EltTypeShift and MemEltTypeShift are initialised twice in one constructor.

If removing them from the member initialisation list throws a warning, perhaps you could use in-class initialisers?

[SjoerdMeijer]

Patches with functional changes but without tests are a bit "suspicious". In this case, I see it might be tricky. You could argue that it should be incorporated in the patch that includes the tests, so we can see what's happening. But perhaps separating this out is cleaner, if the other patch is big. But can you at least make the next patch dependent on this one, so we know where this is used?

[sdesmalen]

In D76617#1938787, @SjoerdMeijer wrote:

Patches with functional changes but without tests are a bit "suspicious". In this case, I see it might be tricky. You could argue that it should be incorporated in the patch that includes the tests, so we can see what's happening. But perhaps separating this out is cleaner, if the other patch is big. But can you at least make the next patch dependent on this one, so we know where this is used?

Yes you're right, the patch that I've made dependent needs this one to work properly.

[SjoerdMeijer]

Yes you're right, the patch that I've made dependent needs this one to work properly.

Ah ok, I may have missed that, will have a look

[sdesmalen]

• Made Flags uint64_t instead of unsigned.
• Simplified encoding of flags in SveEmitter and moved encoding of properties into the Flags variable to the constructor of Intrinsic.
• Added (encoding of) MergeType as well.

[SjoerdMeijer]

clang/utils/TableGen/SveEmitter.cpp

247

Should V now be an uint64_t?

[sdesmalen]

• Updated encode functions to take/return uint64_t instead of unsigned.

[sdesmalen]

clang/utils/TableGen/SveEmitter.cpp

247

Yes, good spot!

[SjoerdMeijer]

Cheers, LGTM

[andwar]

clang/utils/TableGen/SveEmitter.cpp

258

Shouldn't MT be uint64_t instead of unsigned?

[sdesmalen]

Changed unsigned -> uint64_t for encodeMemoryElementType and encodeMergeType.

[andwar]

LGTM

This patch now implements a bit more than the original commit message would suggest. Could you please update? Thanks for working on this!

clang/include/clang/Basic/TargetBuiltins.h

 /// Enumerates target-specific builtins in their own namespaces within
 /// namespace ::clang.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_BASIC_TARGETBUILTINS_H
 #define LLVM_CLANG_BASIC_TARGETBUILTINS_H
 
 #include <stdint.h>
 #include "clang/Basic/Builtins.h"
+#include "llvm/Support/MathExtras.h"
 #undef PPC
 
 namespace clang {
 
   namespace NEON {
   enum {
     LastTIBuiltin = clang::Builtin::FirstTSBuiltin - 1,
 #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
public:
     }
     bool isUnsigned() const { return (Flags & UnsignedFlag) != 0; }
     bool isQuad() const { return (Flags & QuadFlag) != 0; }
   };
 
   /// Flags to identify the types for overloaded SVE builtins.
   class SVETypeFlags {
     uint64_t Flags;
+    unsigned EltTypeShift;
+    unsigned MemEltTypeShift;
 
   public:
 #define LLVM_GET_SVE_TYPEFLAGS
 #include "clang/Basic/arm_sve_typeflags.inc"
 #undef LLVM_GET_SVE_TYPEFLAGS
 
     enum EltType {
 #define LLVM_GET_SVE_ELTTYPES
 #include "clang/Basic/arm_sve_typeflags.inc"
 #undef LLVM_GET_SVE_ELTTYPES
     };
 
     enum MemEltType {
 #define LLVM_GET_SVE_MEMELTTYPES
 #include "clang/Basic/arm_sve_typeflags.inc"
 #undef LLVM_GET_SVE_MEMELTTYPES
     };
 
-    SVETypeFlags(uint64_t F) : Flags(F) {}
-    SVETypeFlags(EltType ET, bool IsUnsigned) : Flags(ET) {}
+    SVETypeFlags(uint64_t F) : Flags(F), EltTypeShift(0), MemEltTypeShift(0) {
+      EltTypeShift = llvm::countTrailingZeros(EltTypeMask);
+      MemEltTypeShift = llvm::countTrailingZeros(MemEltTypeMask);
+    }
 
     EltType getEltType() const {
-      return (EltType)((Flags & EltTypeMask) - FirstEltType);
+      return (EltType)((Flags & EltTypeMask) >> EltTypeShift);
     }
 
     MemEltType getMemEltType() const {
-      return (MemEltType)((Flags & MemEltTypeMask) - FirstMemEltType);
+      return (MemEltType)((Flags & MemEltTypeMask) >> MemEltTypeShift);
     }
 
     bool isLoad() const { return Flags & IsLoad; }
     bool isStore() const { return Flags & IsStore; }
     bool isGatherLoad() const { return Flags & IsGatherLoad; }
     bool isScatterStore() const { return Flags & IsScatterStore; }
     bool isStructLoad() const { return Flags & IsStructLoad; }
     bool isStructStore() const { return Flags & IsStructStore; }

clang/utils/TableGen/SveEmitter.cpp

public:
   /// Emit all the __builtin prototypes and code needed by Sema.
   void createBuiltins(raw_ostream &o);
 
   /// Emit all the information needed to map builtin -> LLVM IR intrinsic.
   void createCodeGenMap(raw_ostream &o);
 
   /// Create the SVETypeFlags used in CGBuiltins
   void createTypeFlags(raw_ostream &o);
 
   /// Create intrinsic and add it to \p Out
   void createIntrinsic(Record *R, SmallVectorImpl<std::unique_ptr<Intrinsic>> &Out);
 };
 
 } // end anonymous namespace
 
 
 //===----------------------------------------------------------------------===//
 // Type implementation
 //===----------------------------------------------------------------------===//
 
 unsigned SVEEmitter::getTypeFlags(const SVEType &T) {
-  unsigned FirstEltType = EltTypes["FirstEltType"];
+  unsigned Shift = llvm::countTrailingZeros(EltTypes["FirstEltType"]);
   if (T.isFloat()) {
     switch (T.getElementSizeInBits()) {
-    case 16: return FirstEltType + EltTypes["EltTyFloat16"];
-    case 32: return FirstEltType + EltTypes["EltTyFloat32"];
-    case 64: return FirstEltType + EltTypes["EltTyFloat64"];
+    case 16: return EltTypes["EltTyFloat16"] << Shift;
+    case 32: return EltTypes["EltTyFloat32"] << Shift;
+    case 64: return EltTypes["EltTyFloat64"] << Shift;
     default: llvm_unreachable("Unhandled float element bitwidth!");
     }
   }
 
   if (T.isPredicateVector()) {
     switch (T.getElementSizeInBits()) {
-    case 8:  return FirstEltType + EltTypes["EltTyBool8"];
-    case 16: return FirstEltType + EltTypes["EltTyBool16"];
-    case 32: return FirstEltType + EltTypes["EltTyBool32"];
-    case 64: return FirstEltType + EltTypes["EltTyBool64"];
+    case 8:  return EltTypes["EltTyBool8"] << Shift;
+    case 16: return EltTypes["EltTyBool16"] << Shift;
+    case 32: return EltTypes["EltTyBool32"] << Shift;
+    case 64: return EltTypes["EltTyBool64"] << Shift;
     default: llvm_unreachable("Unhandled predicate element bitwidth!");
     }
   }
 
   switch (T.getElementSizeInBits()) {
-  case 8:  return FirstEltType + EltTypes["EltTyInt8"];
-  case 16: return FirstEltType + EltTypes["EltTyInt16"];
-  case 32: return FirstEltType + EltTypes["EltTyInt32"];
-  case 64: return FirstEltType + EltTypes["EltTyInt64"];
+  case 8:  return EltTypes["EltTyInt8"] << Shift;
+  case 16: return EltTypes["EltTyInt16"] << Shift;
+  case 32: return EltTypes["EltTyInt32"] << Shift;
+  case 64: return EltTypes["EltTyInt64"] << Shift;
   default: llvm_unreachable("Unhandled integer element bitwidth!");
   }
 }
 
 std::string SVEType::builtin_str() const {
   std::string S;
   if (isVoid())
     return "v";
void SVEEmitter::createIntrinsic(
   StringRef Guard = R->getValueAsString("ArchGuard");
   StringRef LLVMName = R->getValueAsString("LLVMIntrinsic");
   int64_t Merge = R->getValueAsInt("Merge");
   std::vector<Record*> FlagsList = R->getValueAsListOfDefs("Flags");
 
   int64_t Flags = 0;
   for (auto FlagRec : FlagsList)
     Flags |= FlagRec->getValueAsInt("Value");
-  Flags |= R->getValueAsInt("MemEltType") + MemEltTypes["FirstMemEltType"];
+  Flags |= R->getValueAsInt("MemEltType")
+           << llvm::countTrailingZeros(MemEltTypes["FirstMemEltType"]);
 
   // Extract type specs from string
   SmallVector<TypeSpec, 8> TypeSpecs;
   TypeSpec Acc;
   for (char I : Types) {
     Acc.push_back(I);
     if (islower(I)) {
       TypeSpecs.push_back(TypeSpec(Acc));