Switch to using a list of StringAttr

Author: MacDue

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td

@@ -938,15 +938,17 @@ def LLVM_VScaleRangeAttr : LLVM_Attr<"VScaleRange", "vscale_range"> {
 // TargetFeaturesAttr
 //===----------------------------------------------------------------------===//
 
-def LLVM_TargetFeaturesAttr : LLVM_Attr<"TargetFeatures", "target_features"> {
+def LLVM_TargetFeaturesAttr : LLVM_Attr<"TargetFeatures", "target_features">
+{
   let summary = "LLVM target features attribute";
 
   let description = [{
-    Represents the LLVM target features in a manner that is efficient to query.
+    Represents the LLVM target features as a list that can be checked within
+    passes/rewrites.
 
     Example:
     ```mlir
-    #llvm.target_features<"+sme,+sve,+sme-f64f64">
+    #llvm.target_features<["+sme", "+sve", "+sme-f64f64"]>
     ```
 
     Then within a pass or rewrite the features active at an op can be queried:
@@ -959,19 +961,22 @@ def LLVM_TargetFeaturesAttr : LLVM_Attr<"TargetFeatures", "target_features"> {
     ```
   }];
 
-  let parameters = (ins
-    ArrayRefOfSelfAllocationParameter<"TargetFeature", "">:$features);
+  let parameters = (ins OptionalArrayRefParameter<"StringAttr">:$features);
 
   let builders = [
-    TypeBuilder<(ins "::llvm::ArrayRef<TargetFeature>":$features)>,
-    TypeBuilder<(ins "::llvm::StringRef":$features)>
+    TypeBuilder<(ins "::llvm::StringRef":$features)>,
+    TypeBuilder<(ins "::llvm::ArrayRef<::llvm::StringRef>":$features)>
   ];
 
   let extraClassDeclaration = [{
     /// Checks if a feature is contained within the features list.
-    bool contains(TargetFeature) const;
-    bool contains(StringRef feature) const {
-      return contains(TargetFeature{feature});
+    /// Note: Using a StringAttr allows doing pointer-comparisons.
+    bool contains(::mlir::StringAttr feature) const;
+    bool contains(::llvm::StringRef feature) const;
+
+    bool nullOrEmpty() const {
+      // Checks if this attribute is fasly, or the features are empty.
+      return !bool(*this) || getFeatures().empty();
     }
 
     /// Returns the list of features as an LLVM-compatible string.
@@ -987,8 +992,8 @@ def LLVM_TargetFeaturesAttr : LLVM_Attr<"TargetFeatures", "target_features"> {
     }
   }];
 
-  let hasCustomAssemblyFormat = 1;
-  let skipDefaultBuilders = 1;
+  let assemblyFormat = "`<` `[` (`]`) : ($features^ `]`)? `>`";
+  let genVerifyDecl = 1;
 }
 
 #endif // LLVMIR_ATTRDEFS

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h

@@ -74,31 +74,6 @@ class TBAANodeAttr : public Attribute {
   }
 };
 
-/// This struct represents a single LLVM target feature.
-struct TargetFeature {
-  StringRef feature;
-
-  // Support allocating this struct into MLIR storage to ensure the feature
-  // string remains valid.
-  TargetFeature allocateInto(TypeStorageAllocator &alloc) const {
-    return TargetFeature{alloc.copyInto(feature)};
-  }
-
-  operator StringRef() const { return feature; }
-
-  bool operator==(TargetFeature const &other) const {
-    return feature == other.feature;
-  }
-
-  bool operator<(TargetFeature const &other) const {
-    return feature < other.feature;
-  }
-};
-
-inline llvm::hash_code hash_value(const TargetFeature &feature) {
-  return llvm::hash_value(feature.feature);
-}
-
 // Inline the LLVM generated Linkage enum and utility.
 // This is only necessary to isolate the "enum generated code" from the
 // attribute definition itself.

mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp

@@ -19,9 +19,7 @@
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/IR/DebugInfoMetadata.h"
-#include <algorithm>
 #include <optional>
-#include <set>
 
 using namespace mlir;
 using namespace mlir::LLVM;
@@ -191,53 +189,55 @@ void printExpressionArg(AsmPrinter &printer, uint64_t opcode,
 // TargetFeaturesAttr
 //===----------------------------------------------------------------------===//
 
-Attribute TargetFeaturesAttr::parse(mlir::AsmParser &parser, mlir::Type) {
-  std::string targetFeatures;
-  if (parser.parseLess() || parser.parseString(&targetFeatures) ||
-      parser.parseGreater())
-    return {};
-  return get(parser.getContext(), targetFeatures);
-}
-
-void TargetFeaturesAttr::print(mlir::AsmPrinter &printer) const {
-  printer << "<\"";
-  llvm::interleave(
-      getFeatures(), printer,
-      [&](auto &feature) { printer << StringRef(feature); }, ",");
-  printer << "\">";
-}
-
-TargetFeaturesAttr
-TargetFeaturesAttr::get(MLIRContext *context,
-                        llvm::ArrayRef<TargetFeature> featuresRef) {
-  // Sort and de-duplicate target features.
-  std::set<TargetFeature> features(featuresRef.begin(), featuresRef.end());
-  return Base::get(context, llvm::to_vector(features));
+TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
+                                           llvm::ArrayRef<StringRef> features) {
+  return Base::get(context,
+                   llvm::map_to_vector(features, [&](StringRef feature) {
+                     return StringAttr::get(context, feature);
+                   }));
 }
 
 TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
                                            StringRef targetFeatures) {
   SmallVector<StringRef> features;
   targetFeatures.split(features, ',', /*MaxSplit=*/-1,
                        /*KeepEmpty=*/false);
-  return get(context, llvm::map_to_vector(features, [](StringRef feature) {
-               return TargetFeature{feature};
-             }));
+  return get(context, features);
+}
+
+LogicalResult
+TargetFeaturesAttr::verify(function_ref<InFlightDiagnostic()> emitError,
+                           llvm::ArrayRef<StringAttr> features) {
+  for (StringAttr featureAttr : features) {
+    if (!featureAttr || featureAttr.empty())
+      return emitError() << "target features can not be null or empty";
+    auto feature = featureAttr.strref();
+    if (feature[0] != '+' && feature[0] != '-')
+      return emitError() << "target features must start with '+' or '-'";
+    if (feature.contains(','))
+      return emitError() << "target features can not contain ','";
+  }
+  return success();
+}
+
+bool TargetFeaturesAttr::contains(StringAttr feature) const {
+  if (nullOrEmpty())
+    return false;
+  // Note: Using StringAttr does pointer comparisons.
+  return llvm::is_contained(getFeatures(), feature);
 }
 
-bool TargetFeaturesAttr::contains(TargetFeature feature) const {
-  if (!bool(*this))
-    return false; // Allows checking null target features.
-  ArrayRef<TargetFeature> features = getFeatures();
-  // Note: The attribute getter ensures the feature list is sorted.
-  return std::binary_search(features.begin(), features.end(), feature);
+bool TargetFeaturesAttr::contains(StringRef feature) const {
+  if (nullOrEmpty())
+    return false;
+  return llvm::is_contained(getFeatures(), feature);
 }
 
 std::string TargetFeaturesAttr::getFeaturesString() const {
   std::string featuresString;
   llvm::raw_string_ostream ss(featuresString);
   llvm::interleave(
-      getFeatures(), ss, [&](auto &feature) { ss << StringRef(feature); }, ",");
+      getFeatures(), ss, [&](auto &feature) { ss << feature.strref(); }, ",");
   return ss.str();
 }
 

mlir/test/Target/LLVMIR/Import/target-features.ll

@@ -1,7 +1,7 @@
 ; RUN: mlir-translate -import-llvm -split-input-file %s | FileCheck %s
 
 ; CHECK-LABEL: llvm.func @target_features()
-; CHECK-SAME: #llvm.target_features<"+sme,+sme-f64f64,+sve">
+; CHECK-SAME: #llvm.target_features<["+sme", "+sme-f64f64", "+sve"]>
 define void @target_features() #0 {
   ret void
 }

mlir/test/Target/LLVMIR/llvmir-invalid.mlir

@@ -261,6 +261,27 @@ llvm.func @stepvector_intr_wrong_type() -> vector<7xf32> {
 
 // -----
 
+// expected-error @below{{target features can not contain ','}}
+llvm.func @invalid_target_feature() attributes { target_features = #llvm.target_features<["+bad,feature", "+test"]> }
+{
+}
+
+// -----
+
+// expected-error @below{{target features must start with '+' or '-'}}
+llvm.func @missing_target_feature_prefix() attributes { target_features = #llvm.target_features<["sme"]> }
+{
+}
+
+// -----
+
+// expected-error @below{{target features can not be null or empty}}
+llvm.func @empty_target_feature() attributes { target_features = #llvm.target_features<["", "+sve"]> }
+{
+}
+
+// -----
+
 llvm.comdat @__llvm_comdat {
   llvm.comdat_selector @foo any
 }

mlir/test/Target/LLVMIR/target-features.mlir

@@ -1,7 +1,9 @@
 // RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
 
 // CHECK-LABEL: define void @target_features
-// CHECK: attributes #{{.*}} = { "target-features"="+sme,+sme-f64f64,+sve" }
-llvm.func @target_features() attributes { target_features = #llvm.target_features<"+sme,+sve,+sme-f64f64"> } {
+// CHECK: attributes #{{.*}} = { "target-features"="+sme,+sve,+sme-f64f64" }
+llvm.func @target_features() attributes {
+  target_features = #llvm.target_features<["+sme", "+sve", "+sme-f64f64"]>
+} {
   llvm.return
 }