[MLIR][OpenMP] Emit descriptive errors for all unsupported clauses

[skatrak]

This patch improves error reporting in the MLIR to LLVM IR translation pass for the 'omp' dialect by emitting descriptive errors when encountering clauses not yet supported by that pass.

Additionally, not-yet-implemented errors previously missing for some clauses are added, to avoid silently ignoring them.

Error messages related to inlining of omp.private and omp.declare_reduction regions have been updated to use the same format.

[llvmbot]

@llvm/pr-subscribers-mlir-openmp
@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-flang-openmp

Author: Sergio Afonso (skatrak)

Changes

This patch improves error reporting in the MLIR to LLVM IR translation pass for the 'omp' dialect by emitting descriptive errors when encountering clauses not yet supported by that pass.

Additionally, not-yet-implemented errors previously missing for some clauses are added, to avoid silently ignoring them.

Error messages related to inlining of omp.private and omp.declare_reduction regions have been updated to use the same format.

---

Patch is 44.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114037.diff

2 Files Affected:

• (modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (+236-104)
• (modified) mlir/test/Target/LLVMIR/openmp-todo.mlir (+185-27)

diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 1df7b3c083a3a7..fc7d11b33ab242 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -571,7 +571,8 @@ makeReductionGen(omp::DeclareReductionOp decl, llvm::IRBuilderBase &builder,
     if (failed(inlineConvertOmpRegions(decl.getReductionRegion(),
                                        "omp.reduction.nonatomic.body", builder,
                                        moduleTranslation, &phis)))
-      return llvm::createStringError("failed reduction region translation");
+      return llvm::createStringError(
+          "failed to inline `combiner` region of `omp.declare_reduction`");
     assert(phis.size() == 1);
     result = phis[0];
     return builder.saveIP();
@@ -604,7 +605,8 @@ makeAtomicReductionGen(omp::DeclareReductionOp decl,
     if (failed(inlineConvertOmpRegions(decl.getAtomicReductionRegion(),
                                        "omp.reduction.atomic.body", builder,
                                        moduleTranslation, &phis)))
-      return llvm::createStringError("failed reduction region translation");
+      return llvm::createStringError(
+          "failed to inline `atomic` region of `omp.declare_reduction`");
     assert(phis.empty());
     return builder.saveIP();
   };
@@ -640,6 +642,13 @@ convertOmpOrdered(Operation &opInst, llvm::IRBuilderBase &builder,
   return success();
 }
 
+static LogicalResult orderedRegionSupported(omp::OrderedRegionOp op) {
+  if (op.getParLevelSimd())
+    return op.emitError("parallelization-level clause set not yet supported");
+
+  return success();
+}
+
 /// Converts an OpenMP 'ordered_region' operation into LLVM IR using
 /// OpenMPIRBuilder.
 static LogicalResult
@@ -648,9 +657,8 @@ convertOmpOrderedRegion(Operation &opInst, llvm::IRBuilderBase &builder,
   using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
   auto orderedRegionOp = cast<omp::OrderedRegionOp>(opInst);
 
-  // TODO: The code generation for ordered simd directive is not supported yet.
-  if (orderedRegionOp.getParLevelSimd())
-    return opInst.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(orderedRegionSupported(orderedRegionOp)))
+    return failure();
 
   auto bodyGenCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
     // OrderedOp has only one region associated with it.
@@ -717,9 +725,10 @@ allocReductionVars(T loop, ArrayRef<BlockArgument> reductionArgs,
       SmallVector<llvm::Value *, 1> phis;
       if (failed(inlineConvertOmpRegions(allocRegion, "omp.reduction.alloc",
                                          builder, moduleTranslation, &phis)))
-        return failure();
-      assert(phis.size() == 1 && "expected one allocation to be yielded");
+        return loop.emitError(
+            "failed to inline `alloc` region of `omp.declare_reduction`");
 
+      assert(phis.size() == 1 && "expected one allocation to be yielded");
       builder.SetInsertPoint(allocaIP.getBlock()->getTerminator());
 
       // Allocate reduction variable (which is a pointer to the real reduction
@@ -985,6 +994,16 @@ static LogicalResult allocAndInitializeReductionVars(
   return success();
 }
 
+static LogicalResult sectionsOpSupported(omp::SectionsOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op.emitError("allocate clause not yet supported");
+
+  if (!op.getPrivateVars().empty() || op.getPrivateSyms())
+    return op.emitError("privatization clauses not yet supported");
+
+  return success();
+}
+
 static LogicalResult
 convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
                    LLVM::ModuleTranslation &moduleTranslation) {
@@ -994,12 +1013,8 @@ convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
 
   auto sectionsOp = cast<omp::SectionsOp>(opInst);
 
-  // TODO: Support the following clauses: private, firstprivate, lastprivate,
-  // allocate
-  if (!sectionsOp.getAllocateVars().empty() ||
-      !sectionsOp.getAllocatorVars().empty() ||
-      !sectionsOp.getPrivateVars().empty() || sectionsOp.getPrivateSyms())
-    return opInst.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(sectionsOpSupported(sectionsOp)))
+    return failure();
 
   llvm::ArrayRef<bool> isByRef = getIsByRef(sectionsOp.getReductionByref());
   assert(isByRef.size() == sectionsOp.getNumReductionVars());
@@ -1100,6 +1115,16 @@ convertOmpSections(Operation &opInst, llvm::IRBuilderBase &builder,
                                     privateReductionVariables, isByRef);
 }
 
+static LogicalResult singleOpSupported(omp::SingleOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op.emitError("allocate clause not yet supported");
+
+  if (!op.getPrivateVars().empty() || op.getPrivateSyms())
+    return op.emitError("privatization clauses not yet supported");
+
+  return success();
+}
+
 /// Converts an OpenMP single construct into LLVM IR using OpenMPIRBuilder.
 static LogicalResult
 convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
@@ -1107,8 +1132,8 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
   using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
 
-  if (!singleOp.getPrivateVars().empty() || singleOp.getPrivateSyms())
-    return singleOp.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(singleOpSupported(singleOp)))
+    return failure();
 
   auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codegenIP) {
     builder.restoreIP(codegenIP);
@@ -1143,14 +1168,27 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
   return success();
 }
 
+static LogicalResult teamsOpSupported(omp::TeamsOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op.emitError("allocate clause not yet supported");
+
+  if (!op.getPrivateVars().empty() || op.getPrivateSyms())
+    return op.emitError("privatization clauses not yet supported");
+
+  if (!op.getReductionVars().empty() || op.getReductionByref() ||
+      op.getReductionSyms())
+    return op.emitError("reduction clause not yet supported");
+
+  return success();
+}
+
 // Convert an OpenMP Teams construct to LLVM IR using OpenMPIRBuilder
 static LogicalResult
 convertOmpTeams(omp::TeamsOp op, llvm::IRBuilderBase &builder,
                 LLVM::ModuleTranslation &moduleTranslation) {
   using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
-  if (!op.getAllocatorVars().empty() || op.getReductionSyms() ||
-      !op.getPrivateVars().empty() || op.getPrivateSyms())
-    return op.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(teamsOpSupported(op)))
+    return failure();
 
   auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codegenIP) {
     LLVM::ModuleTranslation::SaveStack<OpenMPAllocaStackFrame> frame(
@@ -1215,17 +1253,38 @@ buildDependData(std::optional<ArrayAttr> dependKinds, OperandRange dependVars,
     dds.emplace_back(dd);
   }
 }
+
+static LogicalResult taskOpSupported(omp::TaskOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op->emitError("allocate clause not yet supported");
+
+  if (!op.getInReductionVars().empty() || op.getInReductionByref() ||
+      op.getInReductionSyms())
+    return op.emitError("in_reduction clause not yet supported");
+
+  if (op.getMergeable())
+    return op.emitError("mergeable clause not yet supported");
+
+  if (op.getPriority())
+    return op.emitError("priority clause not yet supported");
+
+  if (!op.getPrivateVars().empty() || op.getPrivateSyms())
+    return op.emitError("privatization clauses not yet supported");
+
+  if (op.getUntied())
+    return op.emitError("untied clause not yet supported");
+
+  return success();
+}
+
 /// Converts an OpenMP task construct into LLVM IR using OpenMPIRBuilder.
 static LogicalResult
 convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
                  LLVM::ModuleTranslation &moduleTranslation) {
   using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
-  if (taskOp.getUntiedAttr() || taskOp.getMergeableAttr() ||
-      taskOp.getInReductionSyms() || taskOp.getPriority() ||
-      !taskOp.getAllocateVars().empty() || !taskOp.getPrivateVars().empty() ||
-      taskOp.getPrivateSyms()) {
-    return taskOp.emitError("unhandled clauses for translation to LLVM IR");
-  }
+  if (failed(taskOpSupported(taskOp)))
+    return failure();
+
   auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codegenIP) {
     // Save the alloca insertion point on ModuleTranslation stack for use in
     // nested regions.
@@ -1258,13 +1317,24 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
   return success();
 }
 
+static LogicalResult taskgroupOpSupported(omp::TaskgroupOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op->emitError("allocate clause not yet supported");
+
+  if (!op.getTaskReductionVars().empty() || op.getTaskReductionByref() ||
+      op.getTaskReductionSyms())
+    return op.emitError("task_reduction clause not yet supported");
+
+  return success();
+}
+
 /// Converts an OpenMP taskgroup construct into LLVM IR using OpenMPIRBuilder.
 static LogicalResult
 convertOmpTaskgroupOp(omp::TaskgroupOp tgOp, llvm::IRBuilderBase &builder,
                       LLVM::ModuleTranslation &moduleTranslation) {
   using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
-  if (!tgOp.getTaskReductionVars().empty() || !tgOp.getAllocateVars().empty())
-    return tgOp.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(taskgroupOpSupported(tgOp)))
+    return failure();
 
   auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codegenIP) {
     builder.restoreIP(codegenIP);
@@ -1286,26 +1356,49 @@ convertOmpTaskgroupOp(omp::TaskgroupOp tgOp, llvm::IRBuilderBase &builder,
   return success();
 }
 
+static LogicalResult taskwaitOpSupported(omp::TaskwaitOp op) {
+  if (!op.getDependVars().empty() || op.getDependKinds())
+    return op.emitError("depend clause not yet supported");
+
+  if (op.getNowait())
+    return op.emitError("nowait clause not yet supported");
+
+  return success();
+}
+
 static LogicalResult
 convertOmpTaskwaitOp(omp::TaskwaitOp twOp, llvm::IRBuilderBase &builder,
                      LLVM::ModuleTranslation &moduleTranslation) {
-  if (!twOp.getDependVars().empty() || twOp.getDependKinds() ||
-      twOp.getNowait())
-    return twOp.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(taskwaitOpSupported(twOp)))
+    return failure();
 
   moduleTranslation.getOpenMPBuilder()->createTaskwait(builder.saveIP());
   return success();
 }
 
+static LogicalResult wsloopOpSupported(omp::WsloopOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op.emitError("allocate clause not yet supported");
+
+  if (!op.getLinearVars().empty() || !op.getLinearStepVars().empty())
+    return op.emitError("linear clause not yet supported");
+
+  if (op.getOrder() || op.getOrderMod())
+    return op.emitError("order clause not yet supported");
+
+  if (!op.getPrivateVars().empty() || op.getPrivateSyms())
+    return op.emitError("privatization clauses not yet supported");
+
+  return success();
+}
+
 /// Converts an OpenMP workshare loop into LLVM IR using OpenMPIRBuilder.
 static LogicalResult
 convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
                  LLVM::ModuleTranslation &moduleTranslation) {
   auto wsloopOp = cast<omp::WsloopOp>(opInst);
-  if (!wsloopOp.getAllocateVars().empty() ||
-      !wsloopOp.getAllocatorVars().empty() ||
-      !wsloopOp.getPrivateVars().empty() || wsloopOp.getPrivateSyms())
-    return opInst.emitError("unhandled clauses for translation to LLVM IR");
+  if (failed(wsloopOpSupported(wsloopOp)))
+    return failure();
 
   // FIXME: Here any other nested wrappers (e.g. omp.simd) are skipped, so
   // codegen for composite constructs like 'DO/FOR SIMD' will be the same as for
@@ -1449,6 +1542,13 @@ convertOmpWsloop(Operation &opInst, llvm::IRBuilderBase &builder,
                                     privateReductionVariables, isByRef);
 }
 
+static LogicalResult parallelOpSupported(omp::ParallelOp op) {
+  if (!op.getAllocateVars().empty() || !op.getAllocatorVars().empty())
+    return op.emitError("allocate clause not yet supported");
+
+  return success();
+}
+
 /// Converts the OpenMP parallel operation to LLVM IR.
 static LogicalResult
 convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
@@ -1458,6 +1558,9 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
   assert(isByRef.size() == opInst.getNumReductionVars());
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
+  if (failed(parallelOpSupported(opInst)))
+    return failure();
+
   // Collect delayed privatization declarations
   MutableArrayRef<BlockArgument> privateBlockArgs =
       cast<omp::BlockArgOpenMPOpInterface>(*opInst).getPrivateBlockArgs();
@@ -1523,8 +1626,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
       if (failed(inlineConvertOmpRegions(allocRegion, "omp.private.alloc",
                                          builder, moduleTranslation, &phis)))
         return llvm::createStringError(
-            "failed to inline `alloc` region of an `omp.private` op in the "
-            "parallel region");
+            "failed to inline `alloc` region of `omp.private`");
 
       assert(phis.size() == 1 && "expected one allocation to be yielded");
 
@@ -1551,7 +1653,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
             opInst, reductionArgs, builder, moduleTranslation, allocaIP,
             reductionDecls, privateReductionVariables, reductionVariableMap,
             deferredStores, isByRef)))
-      return llvm::createStringError("failed reduction vars allocation");
+      return llvm::make_error<SilentTranslationError>();
 
     // Apply copy region for firstprivate.
     bool needsFirstprivate =
@@ -1592,8 +1694,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
       if (failed(inlineConvertOmpRegions(copyRegion, "omp.private.copy",
                                          builder, moduleTranslation)))
         return llvm::createStringError(
-            "failed to inline `copy` region of an `omp.private` op in the "
-            "parallel region");
+            "failed to inline `copy` region of `omp.private`");
 
       // ignore unused value yielded from copy region
 
@@ -1643,8 +1744,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
               reductionDecls[i].getInitializerRegion(), "omp.reduction.neutral",
               builder, moduleTranslation, &phis)))
         return llvm::createStringError(
-            "failed to inline `init` region of an `omp.declare_reduction` op "
-            "in the parallel region");
+            "failed to inline `init` region of `omp.declare_reduction`");
       assert(phis.size() == 1 &&
              "expected one value to be yielded from the "
              "reduction neutral element declaration region");
@@ -1750,8 +1850,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
             reductionCleanupRegions, privateReductionVariables,
             moduleTranslation, builder, "omp.reduction.cleanup")))
       return llvm::createStringError(
-          "failed to inline `cleanup` region of an `omp.declare_reduction` op "
-          "in the parallel region");
+          "failed to inline `cleanup` region of `omp.declare_reduction`");
 
     SmallVector<Region *> privateCleanupRegions;
     llvm::transform(privateDecls, std::back_inserter(privateCleanupRegions),
@@ -1762,8 +1861,8 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
     if (failed(inlineOmpRegionCleanup(
             privateCleanupRegions, llvmPrivateVars, moduleTranslation, builder,
             "omp.private.dealloc", /*shouldLoadCleanupRegionArg=*/false)))
-      return llvm::createStringError("failed to inline `dealloc` region of an "
-                                     "`omp.private` op in the parallel region");
+      return llvm::createStringError(
+          "failed to inline `dealloc` region of `omp.private`");
 
     builder.restoreIP(oldIP);
     return llvm::Error::success();
@@ -1809,9 +1908,15 @@ convertOrderKind(std::optional<omp::ClauseOrderKind> o) {
 }
 
 static LogicalResult simdOpSupported(omp::SimdOp op) {
+  if (!op.getAlignedVars().empty() || op.getAlignments())
+    return op->emitError("aligned clause not yet supported");
+
   if (!op.getLinearVars().empty() || !op.getLinearStepVars().empty())
     return op.emitError("linear clause not yet supported");
 
+  if (!op.getNontemporalVars().empty())
+    return op.emitError("nontemporal clause not yet supported");
+
   if (!op.getPrivateVars().empty() || op.getPrivateSyms())
     return op.emitError("privatization clauses not yet supported");
 
@@ -1939,12 +2044,22 @@ convertAtomicOrdering(std::optional<omp::ClauseMemoryOrderKind> ao) {
   llvm_unreachable("Unknown ClauseMemoryOrderKind kind");
 }
 
+template <typename OpTy>
+static LogicalResult atomicOpSupported(OpTy op) {
+  if (op.getHint())
+    op.emitWarning("hint clause discarded");
+
+  return success();
+}
+
 /// Convert omp.atomic.read operation to LLVM IR.
 static LogicalResult
 convertOmpAtomicRead(Operation &opInst, llvm::IRBuilderBase &builder,
                      LLVM::ModuleTranslation &moduleTranslation) {
-
   auto readOp = cast<omp::AtomicReadOp>(opInst);
+  if (failed(atomicOpSupported(readOp)))
+    return failure();
+
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
@@ -1967,6 +2082,9 @@ static LogicalResult
 convertOmpAtomicWrite(Operation &opInst, llvm::IRBuilderBase &builder,
                       LLVM::ModuleTranslation &moduleTranslation) {
   auto writeOp = cast<omp::AtomicWriteOp>(opInst);
+  if (failed(atomicOpSupported(writeOp)))
+    return failure();
+
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
@@ -2002,6 +2120,8 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
                        llvm::IRBuilderBase &builder,
                        LLVM::ModuleTranslation &moduleTranslation) {
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  if (failed(atomicOpSupported(opInst)))
+    return failure();
 
   // Convert values and types.
   auto &innerOpList = opInst.getRegion().front().getOperations();
@@ -2078,6 +2198,9 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
                         llvm::IRBuilderBase &builder,
                         LLVM::ModuleTranslation &moduleTranslation) {
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
+  if (failed(atomicOpSupported(atomicCaptureOp)))
+    return failure();
+
   mlir::Value mlirExpr;
   bool isXBinopExpr = false, isPostfixUpdate = false;
   llvm::AtomicRMWInst::BinOp binop = llvm::AtomicRMWInst::BinOp::BAD_BINOP;
@@ -3002,6 +3125,14 @@ static void genMapInfos(llvm::IRBuilderBase &builder,
   }
 }
 
+template <typename OpTy>
+static LogicalResult targetDataOpSupported(OpTy op) {
+  if (!op.getDependVars().empty() || op.getDependKinds())
+    return op.emitError("depend clause not yet supported");
+
+  return success();
+}
+
 static LogicalResult
 convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
                      LLVM::ModuleTranslation &moduleTranslation) {
@@ -3034,10 +3165,9 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
             useDeviceAddrVars = dataOp.getUseDeviceAddrVars();
             return success();
           })
-          .Case([&](omp::TargetEnterDataOp enterDataOp) {
-            if (!enterDataOp.getDependVars().empty())
-              return (LogicalResu...
[truncated]

[skatrak]

PR stack:

• [MLIR][OpenMP] Simplify translation to LLVM IR error handling #114036
• [MLIR][OpenMP] Emit descriptive errors for all unsupported clauses #114037

[Meinersbur]

function names should start with a verb: checkWhetherOrderedRegionSupported or maybe checkImplementationStatus.

A Doxygen brief could explain the purpose of this function (not exactly what it is checking, I can see that in the code).

This applies to the helper functions below as well. I don't see the point of extracting out a single check, but seems useful if there are multiple checks and apply the same pattern here as well.

[skatrak]

Thanks for the comment @Meinersbur. I was actually planning on creating a follow-up PR to centralize this a bit more, but I decided that it might help address your comments so I made these changes here. Yes, the idea is to have this be called for all operations regardless of how many unimplemented clauses they have, for consistency. Hopefully the template approach I updated this PR with works for you.

Edit: Perhaps a single function with a type switch is a better alternative to the template for this case, so I think I'll work on that.

[skatrak]

This should be ready to review again. Now the implementation is much more centralized, helping us make sure error messages are consistent.

[Meinersbur]

LGTM

[tblah]

Thanks for this. Looks great!