[clang][RISCV] Drop caching from RVVType as it introduces data races

[kadircet]

Summary

As brought up in https://reviews.llvm.org/D124730#inline-1326968, this
logic is used in places that are supposed to be thread-safe. Unfortunately
putting a single mutex around these maps also wouldn't solve the issue because
RVVType itself is mutable. So I see no other option but to drop the caches &
pass-by-reference logic completely, this likely has performance implications.
Whoever owns this probably should figure out a thread-safe place to store these
instead.

[sammccall]

I think there almost certainly *is* a better way here, but it may be a significant amount of work and we need to get back to a correct state (revert doesn't seem feasible).

Not approving yet in the hope that @kito-cheng (@kito.cheng?) or @khchen can find something better, if we don't hear back I'll stamp in a week or so.

[kito-cheng]

Apparently I missed those comments during llvm dev meeting, I'll figure out a fix soon.

[sammccall]

In D138287#3940602, @kito-cheng wrote:

Apparently I missed those comments during llvm dev meeting, I'll figure out a fix soon.

Thanks a lot!

[kito-cheng]

Proposed fix: D138429

clang/include/clang/Support/RISCVVIntrinsicUtils.h

struct LMULType {
   LMULType(int Log2LMUL);
   // Return the C/C++ string representation of LMUL
   std::string str() const;
   llvm::Optional<unsigned> getScale(unsigned ElementBitwidth) const;
   void MulLog2LMUL(int Log2LMUL);
 };
 
 class RVVType;
-using RVVTypePtr = RVVType *;
-using RVVTypes = std::vector<RVVTypePtr>;
+using RVVTypes = std::vector<RVVType>;
 
 // This class is compact representation of a valid and invalid RVVType.
 class RVVType {
   BasicType BT;
   ScalarTypeKind ScalarType = Invalid;
   LMULType LMUL;
   bool IsPointer = false;
   // IsConstant indices are "int", but have the constant expression.

 public:
   /// Compute output and input types by applying different config (basic type
   /// and LMUL with type transformers). It also record result of type in legal
   /// or illegal set to avoid compute the same config again. The result maybe
   /// have illegal RVVType.
   static llvm::Optional<RVVTypes>
   computeTypes(BasicType BT, int Log2LMUL, unsigned NF,
                llvm::ArrayRef<PrototypeDescriptor> Prototype);
-  static llvm::Optional<RVVTypePtr> computeType(BasicType BT, int Log2LMUL,
-                                                PrototypeDescriptor Proto);
+  static llvm::Optional<RVVType> computeType(BasicType BT, int Log2LMUL,
 };
 
 enum PolicyScheme : uint8_t {
   SchemeNone,
   // Passthru operand is at first parameter in C builtin.
   HasPassthruOperand,
   HasPolicyOperand,
   // Special case for vmerge, the passthru operand is second
private:
   std::string IRName;
   bool IsMasked;
   bool HasMaskedOffOperand;
   bool HasVL;
   PolicyScheme Scheme;
   bool SupportOverloading;
   bool HasBuiltinAlias;
   std::string ManualCodegen;
-  RVVTypePtr OutputType; // Builtin output type
+  RVVType OutputType; // Builtin output type
   RVVTypes InputTypes;   // Builtin input types
   // The types we use to obtain the specific LLVM intrinsic. They are index of
   // InputTypes. -1 means the return type.
   std::vector<int64_t> IntrinsicTypes;
   unsigned NF = 1;
   Policy DefaultPolicy = Policy::PolicyNone;
 
 public:
   RVVIntrinsic(llvm::StringRef Name, llvm::StringRef Suffix,
                llvm::StringRef OverloadedName, llvm::StringRef OverloadedSuffix,
                llvm::StringRef IRName, bool IsMasked, bool HasMaskedOffOperand,
                bool HasVL, PolicyScheme Scheme, bool SupportOverloading,
                bool HasBuiltinAlias, llvm::StringRef ManualCodegen,
                const RVVTypes &Types,
                const std::vector<int64_t> &IntrinsicTypes,
                const std::vector<llvm::StringRef> &RequiredFeatures,
                unsigned NF, Policy DefaultPolicy, bool IsPrototypeDefaultTU);
   ~RVVIntrinsic() = default;
 
-  RVVTypePtr getOutputType() const { return OutputType; }
+  RVVType getOutputType() const { return OutputType; }
   const RVVTypes &getInputTypes() const { return InputTypes; }
   llvm::StringRef getBuiltinName() const { return BuiltinName; }
   llvm::StringRef getName() const { return Name; }
   llvm::StringRef getOverloadedName() const { return OverloadedName; }
   bool hasMaskedOffOperand() const { return HasMaskedOffOperand; }
   bool hasVL() const { return HasVL; }
   bool hasPolicy() const { return Scheme != PolicyScheme::SchemeNone; }
   bool hasPassthruOperand() const {
public:
   // Return the type string for a BUILTIN() macro in Builtins.def.
   std::string getBuiltinTypeStr() const;
 
   static std::string
   getSuffixStr(BasicType Type, int Log2LMUL,
                llvm::ArrayRef<PrototypeDescriptor> PrototypeDescriptors);
 
   static llvm::SmallVector<PrototypeDescriptor>
-      computeBuiltinTypes(llvm::ArrayRef<PrototypeDescriptor> Prototype,
-                          bool IsMasked, bool HasMaskedOffOperand, bool HasVL,
-                          unsigned NF, bool IsPrototypeDefaultTU,
-                          PolicyScheme DefaultScheme,
-                          Policy DefaultPolicy = Policy::PolicyNone);
   static llvm::SmallVector<Policy>
-      getSupportedMaskedPolicies(bool HasTailPolicy, bool HasMaskPolicy);
 
   static void updateNamesAndPolicy(bool IsMasked, bool HasPolicy,
                                    bool IsPrototypeDefaultTU, std::string &Name,
                                    std::string &BuiltinName,
                                    std::string &OverloadedName,
                                    Policy &DefaultPolicy);
 };
 

clang/lib/Sema/SemaRISCVVectorLookup.cpp

 
 void RISCVIntrinsicManagerImpl::CreateRVVIntrinsicDecl(LookupResult &LR,
                                                        IdentifierInfo *II,
                                                        Preprocessor &PP,
                                                        unsigned Index,
                                                        bool IsOverload) {
   ASTContext &Context = S.Context;
   RVVIntrinsicDef &IDef = IntrinsicList[Index];
-  RVVTypes Sigs = IDef.Signature;
+  RVVTypes &Sigs = IDef.Signature;
   size_t SigLength = Sigs.size();
-  RVVType *ReturnType = Sigs[0];
-  QualType RetType = RVVType2Qual(Context, ReturnType);
+  QualType RetType = RVVType2Qual(Context, &Sigs[0]);
   SmallVector<QualType, 8> ArgTypes;
   QualType BuiltinFuncType;
 
   // Skip return type, and convert RVVType to QualType for arguments.
   for (size_t i = 1; i < SigLength; ++i)
-    ArgTypes.push_back(RVVType2Qual(Context, Sigs[i]));
+    ArgTypes.push_back(RVVType2Qual(Context, &Sigs[i]));
 
   FunctionProtoType::ExtProtoInfo PI(
       Context.getDefaultCallingConvention(false, false, true));
 
   PI.Variadic = false;
 
   SourceLocation Loc = LR.getNameLoc();
   BuiltinFuncType = Context.getFunctionType(RetType, ArgTypes, PI);

clang/lib/Support/RISCVVIntrinsicUtils.cpp

for (const PrototypeDescriptor &Proto : Prototype) {
     if (!T)
       return llvm::None;
     // Record legal type index
     Types.push_back(T.value());
   }
   return Types;
 }
 
-// Compute the hash value of RVVType, used for cache the result of computeType.
-static uint64_t computeRVVTypeHashValue(BasicType BT, int Log2LMUL,
+Optional<RVVType> RVVType::computeType(BasicType BT, int Log2LMUL,
                                           PrototypeDescriptor Proto) {
-  // Layout of hash value:
-  // 0               8    16          24        32          40
-  // | Log2LMUL + 3  | BT  | Proto.PT | Proto.TM | Proto.VTM |
-  assert(Log2LMUL >= -3 && Log2LMUL <= 3);
-  return (Log2LMUL + 3) | (static_cast<uint64_t>(BT) & 0xff) << 8 |
-         ((uint64_t)(Proto.PT & 0xff) << 16) |
-         ((uint64_t)(Proto.TM & 0xff) << 24) |
-         ((uint64_t)(Proto.VTM & 0xff) << 32);
-}
-
-Optional<RVVTypePtr> RVVType::computeType(BasicType BT, int Log2LMUL,
-                                          PrototypeDescriptor Proto) {
-  // Concat BasicType, LMUL and Proto as key
-  static std::unordered_map<uint64_t, RVVType> LegalTypes;
-  static std::set<uint64_t> IllegalTypes;
-  uint64_t Idx = computeRVVTypeHashValue(BT, Log2LMUL, Proto);
-  // Search first
-  auto It = LegalTypes.find(Idx);
-  if (It != LegalTypes.end())
-    return &(It->second);
-
-  if (IllegalTypes.count(Idx))
-    return llvm::None;
-
-  // Compute type and record the result.
   RVVType T(BT, Log2LMUL, Proto);
-  if (T.isValid()) {
-    // Record legal type index and value.
-    LegalTypes.insert({Idx, T});
-    return &(LegalTypes[Idx]);
-  }
-  // Record illegal type index.
-  IllegalTypes.insert(Idx);
+  if (T.isValid())
+    return T;
   return llvm::None;
 }
 
 //===----------------------------------------------------------------------===//
 // RVVIntrinsic implementation
 //===----------------------------------------------------------------------===//
 RVVIntrinsic::RVVIntrinsic(
     StringRef NewName, StringRef Suffix, StringRef NewOverloadedName,
for (auto &I : IntrinsicTypes) {
       if (I >= 0)
         I += NF;
     }
   }
 }
 
 std::string RVVIntrinsic::getBuiltinTypeStr() const {
   std::string S;
-  S += OutputType->getBuiltinStr();
+  S += OutputType.getBuiltinStr();
   for (const auto &T : InputTypes) {
-    S += T->getBuiltinStr();
+    S += T.getBuiltinStr();
   }
   return S;
 }
 
 std::string RVVIntrinsic::getSuffixStr(
     BasicType Type, int Log2LMUL,
     llvm::ArrayRef<PrototypeDescriptor> PrototypeDescriptors) {
   SmallVector<std::string> SuffixStrs;
   for (auto PD : PrototypeDescriptors) {
     auto T = RVVType::computeType(Type, Log2LMUL, PD);
-    SuffixStrs.push_back((*T)->getShortStr());
+    SuffixStrs.push_back((*T).getShortStr());
   }
   return join(SuffixStrs, "_");
 }
 
 llvm::SmallVector<PrototypeDescriptor> RVVIntrinsic::computeBuiltinTypes(
     llvm::ArrayRef<PrototypeDescriptor> Prototype, bool IsMasked,
     bool HasMaskedOffOperand, bool HasVL, unsigned NF,
     bool IsPrototypeDefaultTU, PolicyScheme DefaultScheme,

clang/utils/TableGen/RISCVVEmitter.cpp

if (RVVI->hasManualCodegen()) {
     OS << "  DefaultPolicy = " << RVVI->getDefaultPolicyBits() << ";\n";
     OS << RVVI->getManualCodegen();
     OS << "break;\n";
     return;
   }
 
   // Cast pointer operand of vector load intrinsic.
   for (const auto &I : enumerate(RVVI->getInputTypes())) {
-    if (I.value()->isPointer()) {
+    if (I.value().isPointer()) {
       assert(RVVI->getIntrinsicTypes().front() == -1 &&
              "RVVI should be vector load intrinsic.");
       OS << "  Ops[" << I.index() << "] = Builder.CreateBitCast(Ops[";
       OS << I.index() << "], ResultType->getPointerTo());\n";
     }
   }
 
   if (RVVI->isMasked()) {
void RVVEmitter::createHeader(raw_ostream &OS) {
   OS << "extern \"C\" {\n";
   OS << "#endif\n\n";
 
   OS << "#pragma clang riscv intrinsic vector\n\n";
 
   printHeaderCode(OS);
 
   auto printType = [&](auto T) {
-    OS << "typedef " << T->getClangBuiltinStr() << " " << T->getTypeStr()
+    OS << "typedef " << T.getClangBuiltinStr() << " " << T.getTypeStr()
        << ";\n";
   };
 
   constexpr int Log2LMULs[] = {-3, -2, -1, 0, 1, 2, 3};
   // Print RVV boolean types.
   for (int Log2LMUL : Log2LMULs) {
     auto T = RVVType::computeType(BasicType::Int8, Log2LMUL,
                                   PrototypeDescriptor::Mask);