[mlir][llvmir] implement missing attrs getChecked

[makslevental]

These are declared but not implemented in LLVMAttrDefs.td. This has gone unnoticed because the linker DCEs the decl unless someone accidentally tries to actually call those functions...

[llvmbot]

@llvm/pr-subscribers-mlir-llvm

Author: Maksim Levental (makslevental)

Changes

These are declared but not implemented in LLVMAttrDefs.td. This has gone unnoticed because the linker DCEs the decl unless someone accidentally tries to actually call those functions...

---

Full diff: https://github.com/llvm/llvm-project/pull/121248.diff

1 Files Affected:

• (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp (+23)

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index c7ddc1b36f4d4f..6823bf05d1e2d8 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -288,6 +288,15 @@ TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
                    }));
 }
 
+TargetFeaturesAttr TargetFeaturesAttr::getChecked(
+    llvm::function_ref<::mlir::InFlightDiagnostic()> emitError,
+    MLIRContext *context, llvm::ArrayRef<StringRef> features) {
+  return Base::getChecked(emitError, context,
+                          llvm::map_to_vector(features, [&](StringRef feature) {
+                            return StringAttr::get(context, feature);
+                          }));
+}
+
 TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
                                            StringRef targetFeatures) {
   SmallVector<StringRef> features;
@@ -296,6 +305,20 @@ TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
   return get(context, features);
 }
 
+TargetFeaturesAttr TargetFeaturesAttr::getChecked(
+    llvm::function_ref<::mlir::InFlightDiagnostic()> emitError,
+    MLIRContext *context, StringRef targetFeatures) {
+  SmallVector<StringRef> features;
+  targetFeatures.split(features, ',', /*MaxSplit=*/-1,
+                       /*KeepEmpty=*/false);
+  SmallVector<StringAttr> featuresAttrs;
+  featuresAttrs.reserve(features.size());
+  for (StringRef feature : features) {
+    featuresAttrs.push_back(StringAttr::get(context, feature));
+  }
+  return getChecked(emitError, context, featuresAttrs);
+}
+
 LogicalResult
 TargetFeaturesAttr::verify(function_ref<InFlightDiagnostic()> emitError,
                            llvm::ArrayRef<StringAttr> features) {

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

These are declared but not implemented in LLVMAttrDefs.td. This has gone unnoticed because the linker DCEs the decl unless someone accidentally tries to actually call those functions...

---

Full diff: https://github.com/llvm/llvm-project/pull/121248.diff

1 Files Affected:

• (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp (+23)

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index c7ddc1b36f4d4f..6823bf05d1e2d8 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -288,6 +288,15 @@ TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
                    }));
 }
 
+TargetFeaturesAttr TargetFeaturesAttr::getChecked(
+    llvm::function_ref<::mlir::InFlightDiagnostic()> emitError,
+    MLIRContext *context, llvm::ArrayRef<StringRef> features) {
+  return Base::getChecked(emitError, context,
+                          llvm::map_to_vector(features, [&](StringRef feature) {
+                            return StringAttr::get(context, feature);
+                          }));
+}
+
 TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
                                            StringRef targetFeatures) {
   SmallVector<StringRef> features;
@@ -296,6 +305,20 @@ TargetFeaturesAttr TargetFeaturesAttr::get(MLIRContext *context,
   return get(context, features);
 }
 
+TargetFeaturesAttr TargetFeaturesAttr::getChecked(
+    llvm::function_ref<::mlir::InFlightDiagnostic()> emitError,
+    MLIRContext *context, StringRef targetFeatures) {
+  SmallVector<StringRef> features;
+  targetFeatures.split(features, ',', /*MaxSplit=*/-1,
+                       /*KeepEmpty=*/false);
+  SmallVector<StringAttr> featuresAttrs;
+  featuresAttrs.reserve(features.size());
+  for (StringRef feature : features) {
+    featuresAttrs.push_back(StringAttr::get(context, feature));
+  }
+  return getChecked(emitError, context, featuresAttrs);
+}
+
 LogicalResult
 TargetFeaturesAttr::verify(function_ref<InFlightDiagnostic()> emitError,
                            llvm::ArrayRef<StringAttr> features) {

[gysit]

Thanks for the fix!

LGTM modulo comments if they apply.

[gysit]

Suggested change

	SmallVector<StringAttr> featuresAttrs;
	featuresAttrs.reserve(features.size());
	for (StringRef feature : features) {
	featuresAttrs.push_back(StringAttr::get(context, feature));
	}
	return getChecked(emitError, context, featuresAttrs);
	return getChecked(emitError, context, features);

Couldn't we simplify this and use the getChecked signature you defined above? That way we avoid duplicating the logic that converts from a StringRef to a StringAttr.

[makslevental]

I don't know if I was asleep or what when I did this.................

[makslevental]

Now I remember - for some reason that correct ArrayRef(SmallVector) ctor wasn't being deduced so it was going all the way to StorageBase (which then winds its way back to the ArrayRef<StringAttr> builder. Adding ArrayRef featuresRef(features) fixed.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.