[mlir][sparse] Requiring emitCInterface parameter to be explicit

[wrengr]

Summary

Depends On D115004

Cleans up code legibility by requiring the emitCInterface parameter to be explicit at all call-sites, and defining boolean aliases for that parameter.

[rriddle]

Please include justification for patches in the description.

[mehdi_amini]

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

771

771

(same below)

[wrengr]

addressing comments

[aartbik]

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.
So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

[wrengr]

In D115005#3170000, @aartbik wrote:

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.

I was thinking that might be the case, but wasn't certain. This differential just came about from golfing with the code (alongside D115004, D115008, D115010, and D115012, before I split them up into manageable chunks). So I'm not super attached to it, if you and River are opposed to the change.

So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

So we should keep the /*emitCInterface=*/ even if there's no default? So then I'm guessing the reason for the annotation is because of the bool type rather than because of the optionality (as I'd been thinking)? Is there a style guide anywhere that covers these sorts of things? (since I don't recall seeing it in the llvm nor google guides)

[wrengr]

Adding aliases for bool, and removing the default for emitCInterface.

[aartbik]

The default was chosen since emitCInterface is the exception when generating MLIR functions, even if the majority of our here require it.
So, let's not use defaults then, and make it explicit everywhere. As Mehdi also commented, and as I asked in the other revision
please keep /*emitCInterface=*/everywhere

(or alternatively create a CINTERFACE_ON/CINTERFACE/OFF alias for true/false, but that may be overkill here)

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

155

For a moment, I was afraid this goes against our style, but browsing around, I see all sort of conventions for enum constants, so if lint does not complain, I guess it is okay.

Still, move enum declaration to top please, before any methods.

[wrengr]

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

155

The linter is cool with it.

If you'd rather I use some other approach (e.g., different names, enum representation, #define in lieu of enum, etc), just let me know. I just went with the first thing that came to mind and the names you suggested :)

[wrengr]

Moving the enum definition to the top

[aartbik]

New code reads nicely. Thanks!

[rriddle]

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

36

nit: Seems safer to use a proper enum class (and more inline with coding style)

769

Why pull out all of these adaptor.getOperands()?

[wrengr]

changing enum to enum class

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

769

To avoid linewrapping is all. Same reason for doing auto call = createFuncCall(...); return call.getResult(0) all over. Though with the longer names of the enum class variation, a bunch of stuff is going to have to linewrap anyways. Then again, re-running it just now, it looks like some change affected the linter's baroque heuristics for how to linewrap things, so the results aren't as ugly as last time I tried leaving the adaptor.getOperands() inline

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp

 #include "mlir/ExecutionEngine/SparseTensorUtils.h"
 #include "mlir/Transforms/DialectConversion.h"
 
 using namespace mlir;
 using namespace mlir::sparse_tensor;
 
 namespace {
 
+/// Shorthand aliases for the `emitCInterface` argument to `getFunc()`,
+/// `createFuncCall()`, and `replaceOpWithFuncCall()`.
+enum EmitCInterface : bool { CINTERFACE_ON = true, CINTERFACE_OFF = false };
+
 //===----------------------------------------------------------------------===//
 // Helper methods.
 //===----------------------------------------------------------------------===//
 
 /// Generates a constant zero of the given type.
 inline static Value constantZero(ConversionPatternRewriter &rewriter,
                                  Location loc, Type t) {
   return rewriter.create<arith::ConstantOp>(loc, t, rewriter.getZeroAttr(t));

 }
 
 /// Returns the equivalent of `void*` for opaque arguments to the
 /// execution engine.
 static Type getOpaquePointerType(PatternRewriter &rewriter) {
   return LLVM::LLVMPointerType::get(rewriter.getI8Type());
 }
 
 /// Returns a function reference (first hit also inserts into module). Sets
 /// the "_emit_c_interface" on the function declaration when requested,
 /// so that LLVM lowering generates a wrapper function that takes care
 /// of ABI complications with passing in and returning MemRefs to C functions.
 static FlatSymbolRefAttr getFunc(Operation *op, StringRef name,
                                  TypeRange resultType, ValueRange operands,
-                                 bool emitCInterface) {
+                                 EmitCInterface emitCInterface) {
   MLIRContext *context = op->getContext();
   auto module = op->getParentOfType<ModuleOp>();
   auto result = SymbolRefAttr::get(context, name);
   auto func = module.lookupSymbol<FuncOp>(result.getAttr());
   if (!func) {
     OpBuilder moduleBuilder(module.getBodyRegion());
     func = moduleBuilder.create<FuncOp>(
         op->getLoc(), name,
         FunctionType::get(context, operands.getTypes(), resultType));
     func.setPrivate();
     if (emitCInterface)
       func->setAttr("llvm.emit_c_interface", UnitAttr::get(context));
   }
   return result;
 }
 
 /// Creates a `CallOp` to the function reference returned by `getFunc()`.
 static CallOp createFuncCall(OpBuilder &builder, Operation *op, StringRef name,
                              TypeRange resultType, ValueRange operands,
-                             bool emitCInterface = false) {
+                             EmitCInterface emitCInterface) {
   auto fn = getFunc(op, name, resultType, operands, emitCInterface);
   return builder.create<CallOp>(op->getLoc(), resultType, fn, operands);
 }
 
 /// Replaces the `op` with  a `CallOp` to the function reference returned
 /// by `getFunc()`.
 static CallOp replaceOpWithFuncCall(PatternRewriter &rewriter, Operation *op,
                                     StringRef name, TypeRange resultType,
                                     ValueRange operands,
-                                    bool emitCInterface = false) {
+                                    EmitCInterface emitCInterface) {
   auto fn = getFunc(op, name, resultType, operands, emitCInterface);
   return rewriter.replaceOpWithNewOp<CallOp>(op, resultType, fn, operands);
 }
 
 /// Generates dimension size call.
 static Value genDimSizeCall(ConversionPatternRewriter &rewriter, Operation *op,
                             SparseTensorEncodingAttr &enc, Value src,
                             int64_t idx) {
   // Permute the index according to an optional dimension ordering.
   if (AffineMap p = enc.getDimOrdering())
     idx = p.getPermutedPosition(idx);
   // Generate the call.
   StringRef name = "sparseDimSize";
   SmallVector<Value, 2> params{src, constantIndex(rewriter, op->getLoc(), idx)};
   Type iTp = rewriter.getIndexType();
-  return createFuncCall(rewriter, op, name, iTp, params).getResult(0);
+  auto call = createFuncCall(rewriter, op, name, iTp, params, CINTERFACE_OFF);
+  return call.getResult(0);
 }
 
 /// Generates a call into the "swiss army knife" method of the sparse runtime
 /// support library for materializing sparse tensors into the computation.
 static Value genNewCall(ConversionPatternRewriter &rewriter, Operation *op,
                         ArrayRef<Value> params) {
   StringRef name = "newSparseTensor";
   Type pTp = getOpaquePointerType(rewriter);
-  auto call = createFuncCall(rewriter, op, name, pTp, params,
-                             /*emitCInterface=*/true);
+  auto call = createFuncCall(rewriter, op, name, pTp, params, CINTERFACE_ON);
   return call.getResult(0);
 }
 
 /// Populates given sizes array from type.
 static void sizesFromType(ConversionPatternRewriter &rewriter,
                           SmallVector<Value, 4> &sizes, Location loc,
                           ShapedType stp) {
   auto shape = stp.getShape();
static void genAddEltCall(ConversionPatternRewriter &rewriter, Operation *op,
   else if (eltType.isInteger(16))
     name = "addEltI16";
   else if (eltType.isInteger(8))
     name = "addEltI8";
   else
     llvm_unreachable("Unknown element type");
   SmallVector<Value, 4> params{ptr, val, ind, perm};
   Type pTp = getOpaquePointerType(rewriter);
-  createFuncCall(rewriter, op, name, pTp, params, /*emitCInterface=*/true);
+  createFuncCall(rewriter, op, name, pTp, params, CINTERFACE_ON);
 }
 
 /// Generates a call to `iter->getNext()`.  If there is a next element,
 /// then it is copied into the out-parameters `ind` and `elemPtr`,
 /// and the return value is true.  If there isn't a next element, then
 /// the memory for `iter` is freed and the return value is false.
 static Value genGetNextCall(ConversionPatternRewriter &rewriter, Operation *op,
                             Value iter, Value ind, Value elemPtr) {
static Value genGetNextCall(ConversionPatternRewriter &rewriter, Operation *op,
   else if (elemTp.isInteger(16))
     name = "getNextI16";
   else if (elemTp.isInteger(8))
     name = "getNextI8";
   else
     llvm_unreachable("Unknown element type");
   SmallVector<Value, 3> params{iter, ind, elemPtr};
   Type i1 = rewriter.getI1Type();
-  auto call = createFuncCall(rewriter, op, name, i1, params,
-                             /*emitCInterface=*/true);
+  auto call = createFuncCall(rewriter, op, name, i1, params, CINTERFACE_ON);
   return call.getResult(0);
 }
 
 /// If the tensor is a sparse constant, generates and returns the pair of
 /// the constants for the indices and the values.
 static Optional<std::pair<Value, Value>>
 genSplitSparseConstant(ConversionPatternRewriter &rewriter, Location loc,
                        Value tensor) {

 /// Sparse conversion rule for the release operator.
 class SparseTensorReleaseConverter : public OpConversionPattern<ReleaseOp> {
 public:
   using OpConversionPattern::OpConversionPattern;
   LogicalResult
   matchAndRewrite(ReleaseOp op, OpAdaptor adaptor,
                   ConversionPatternRewriter &rewriter) const override {
     StringRef name = "delSparseTensor";
+    auto params = adaptor.getOperands();
     TypeRange noTp;
-    createFuncCall(rewriter, op, name, noTp, adaptor.getOperands());
+    createFuncCall(rewriter, op, name, noTp, params, CINTERFACE_OFF);
     rewriter.eraseOp(op);
     return success();
   }
 };
 
 /// Sparse conversion rule for pointer accesses.
 class SparseTensorToPointersConverter
     : public OpConversionPattern<ToPointersOp> {
matchAndRewrite(ToPointersOp op, OpAdaptor adaptor,
     else if (eltType.isInteger(32))
       name = "sparsePointers32";
     else if (eltType.isInteger(16))
       name = "sparsePointers16";
     else if (eltType.isInteger(8))
       name = "sparsePointers8";
     else
       return failure();
-    replaceOpWithFuncCall(rewriter, op, name, resType, adaptor.getOperands(),
-                          /*emitCInterface=*/true);
+    auto params = adaptor.getOperands();
+    replaceOpWithFuncCall(rewriter, op, name, resType, params, CINTERFACE_ON);
     return success();
   }
 };
 
 /// Sparse conversion rule for index accesses.
 class SparseTensorToIndicesConverter : public OpConversionPattern<ToIndicesOp> {
 public:
   using OpConversionPattern::OpConversionPattern;
matchAndRewrite(ToIndicesOp op, OpAdaptor adaptor,
     else if (eltType.isInteger(32))
       name = "sparseIndices32";
     else if (eltType.isInteger(16))
       name = "sparseIndices16";
     else if (eltType.isInteger(8))
       name = "sparseIndices8";
     else
       return failure();
-    replaceOpWithFuncCall(rewriter, op, name, resType, adaptor.getOperands(),
-                          /*emitCInterface=*/true);
+    auto params = adaptor.getOperands();
+    replaceOpWithFuncCall(rewriter, op, name, resType, params, CINTERFACE_ON);
     return success();
   }
 };
 
 /// Sparse conversion rule for value accesses.
 class SparseTensorToValuesConverter : public OpConversionPattern<ToValuesOp> {
 public:
   using OpConversionPattern::OpConversionPattern;
matchAndRewrite(ToValuesOp op, OpAdaptor adaptor,
     else if (eltType.isInteger(32))
       name = "sparseValuesI32";
     else if (eltType.isInteger(16))
       name = "sparseValuesI16";
     else if (eltType.isInteger(8))
       name = "sparseValuesI8";
     else
       return failure();
-    replaceOpWithFuncCall(rewriter, op, name, resType, adaptor.getOperands(),
-                          /*emitCInterface=*/true);
+    auto params = adaptor.getOperands();
+    replaceOpWithFuncCall(rewriter, op, name, resType, params, CINTERFACE_ON);
     return success();
   }
 };
 
 /// Sparse conversion rule for tensor rematerialization.
 class SparseTensorLoadConverter : public OpConversionPattern<LoadOp> {
 public:
   using OpConversionPattern::OpConversionPattern;
   LogicalResult
   matchAndRewrite(LoadOp op, OpAdaptor adaptor,
                   ConversionPatternRewriter &rewriter) const override {
     if (op.hasInserts()) {
       // Finalize any pending insertions.
       StringRef name = "endInsert";
+      auto params = adaptor.getOperands();
       TypeRange noTp;
-      createFuncCall(rewriter, op, name, noTp, adaptor.getOperands());
+      createFuncCall(rewriter, op, name, noTp, params, CINTERFACE_OFF);
     }
     rewriter.replaceOp(op, adaptor.getOperands());
     return success();
   }
 };
 
 /// Sparse conversion rule for inserting in lexicographic index order.
 class SparseTensorLexInsertConverter : public OpConversionPattern<LexInsertOp> {
matchAndRewrite(LexInsertOp op, OpAdaptor adaptor,
     else if (eltType.isInteger(32))
       name = "lexInsertI32";
     else if (eltType.isInteger(16))
       name = "lexInsertI16";
     else if (eltType.isInteger(8))
       name = "lexInsertI8";
     else
       llvm_unreachable("Unknown element type");
+    auto params = adaptor.getOperands();
     TypeRange noTp;
-    replaceOpWithFuncCall(rewriter, op, name, noTp, adaptor.getOperands(),
-                          /*emitCInterface=*/true);
+    replaceOpWithFuncCall(rewriter, op, name, noTp, params, CINTERFACE_ON);
     return success();
   }
 };
 
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // Public method for populating conversion rules.