[mlir][Linalg] Inline an interface method to its only user.

[akuegel]

Summary

It seems only the default implementation is ever used, so it doesn't seem
necessary to include this method in the interface.

[ftynse]

There are plenty of cases where only the default implementation is used that we can switch from being an (overridable) interface method to an extraSharedClassDeclaration on the interface class IMO.

[akuegel]

In D130986#3693451, @ftynse wrote:

There are plenty of cases where only the default implementation is used that we can switch from being an (overridable) interface method to an extraSharedClassDeclaration on the interface class IMO.

Agree, I think there are more. This was just one method that I hit when trying to extract the interface for destination-style ops (discussed in https://discourse.llvm.org/t/rfc-interface-for-destination-style-ops/64056)
While working on that, I am happy to do further cleanups.

[mravishankar]

I am not sure we want to do this. Some of this was implemented this way to hide the fact that the input and output operands of a linalg op being consecutive is am implementation detail of current Linalg op definition, and would like to hide this from users by relying on interface methods to querry the information without relying on operand ordering.

[akuegel]

In D130986#3694491, @mravishankar wrote:

I am not sure we want to do this. Some of this was implemented this way to hide the fact that the input and output operands of a linalg op being consecutive is am implementation detail of current Linalg op definition, and would like to hide this from users by relying on interface methods to querry the information without relying on operand ordering.

I don't understand this concern. Inlining the method still allows to provide custom getInputOperands and getOutputOperands interface methods which do not need to meet the assumption that the input and output operands are consecutive. And regarding the assumption that the output operands appear consecutive, that is already there in the code, as the method is only allowed to return two values: begin and end of where the output positions are in the loops to shapes map.

[pifon2a]

In D130986#3694491, @mravishankar wrote:

I am not sure we want to do this. Some of this was implemented this way to hide the fact that the input and output operands of a linalg op being consecutive is am implementation detail of current Linalg op definition, and would like to hide this from users by relying on interface methods to querry the information without relying on operand ordering.

I am not sure I follow. This PR does not really change anything. Some of the interfaces in Linalg have to be revisited and cleaned up.

@akuegel, I like @ftynse's idea to just move it to extraSharedClassDeclaration or even make it a static func in cpp file, just to keep stuff readable.

[akuegel]

Move to static function.

[akuegel]

Fix formatting.

[mravishankar]

Change now makes sense since it is limited to LinalgInterface. This was a a bit of frought API to begin with (full disclosure, I added it).

[akuegel]

In D130986#3696887, @mravishankar wrote:

Change now makes sense since it is limited to LinalgInterface. This was a a bit of frought API to begin with (full disclosure, I added it).

Just for the record, it was already limited to LinalgInterface before. The only thing I changed is that I have extracted it into a static function.

[pifon2a]

In D130986#3696887, @mravishankar wrote:

Change now makes sense since it is limited to LinalgInterface. This was a a bit of frought API to begin with (full disclosure, I added it).

I don't understand. How is it different from just inlining that code? It was already limited to LinalgInterface. Why didn't you approve it before? What changed other than the readability aspect?

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td

InterfaceMethod<
       /*args=*/(ins "ArrayRef<OpOperand *>":$droppedOperands),
       /*methodBody=*/"",
       /*defaultImplementation=*/[{
         return detail::canOpOperandsBeDroppedImpl($_op, droppedOperands);
       }]
     >,
     InterfaceMethod<
       /*desc=*/[{
-        Return the range of position in the result of the affine map
-        computed by getLoopsToShapesMap() which correspond to the
-        AffineExprs used to access the outputs of the operation.
-      }],
-      /*retTy=*/"std::pair<int64_t, int64_t>",
-      /*methodName=*/"getResultsPositionInLoopsToShapeMap",
-      /*args=*/(ins),
-      /*methodBody=*/"",
-      /*defaultImplementation=*/[{
-        int64_t inputRankSum = 0;
-        int64_t outputRankSum = 0;
-        for(OpOperand *input : getInputOperands())
-          inputRankSum += getRank(input);
-        for(OpOperand *output : getOutputOperands())
-          outputRankSum += getRank(output);
-        return {inputRankSum, inputRankSum + outputRankSum};
-      }]
-    >,
-    InterfaceMethod<
-      /*desc=*/[{
         Like `getShape`, but only returns statically-known information, without
         generating any new IR. For each shape dimension, returns >=0 if that
         dimension is statically known, or ShapeType::kDynamicSize otherwise.
       }],
       /*retTy=*/"SmallVector<int64_t>",
       /*methodName=*/"getStaticShape",
       /*args=*/(ins),
       /*methodBody=*/"",

mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp

struct HasAffineDimExprVisitor
   bool visitConstantExpr(AffineConstantExpr constExpr) { return false; }
 
   bool visitSymbolExpr(AffineSymbolExpr symbolExpr) { return false; }
 
 private:
   llvm::SmallBitVector positions;
 };
 
+static std::pair<int64_t, int64_t>
+getResultsPositionInLoopsToShapeMap(LinalgOp &op) {
+  int64_t inputRankSum = 0;
+  int64_t outputRankSum = 0;
+  for (OpOperand *input : op.getInputOperands())
+    inputRankSum += op.getRank(input);
+  for (OpOperand *output : op.getOutputOperands())
+    outputRankSum += op.getRank(output);
+  return {inputRankSum, inputRankSum + outputRankSum};
+}
+
 LogicalResult
 LinalgOp::reifyResultShapes(OpBuilder &b,
                             ReifiedRankedShapedTypeDims &reifiedReturnShapes) {
   // An example that helps understand the logic below.
   // Consider the following expression O(i+j, j) += A(i,k) * B(k, j)
   // We want to express the shape of dim 0 of O in terms of shape of the inputs.
   // This is achieved as follows.
   //   loopsToShapesMap = (d0, d1, d2) -> (d0, d2, d2, d1, d0 + d1, d1)
   //   subMapOfResultShapes = (d0, d1, d2) -> (d0 + d1, d1)
   //   shapesToLoopsMap = (d0, d2, d2, d3, d4, d5) -> (d0, d3, d2)
   //   resultShapesFromInputShapes = subMapOfResultDim.compose(shapesToLoopMap)
   //     = (d0, d1, d2, d3, d4, d5) -> (d0 + d1, d1)
   AffineMap loopsToShapesMap = getLoopsToShapesMap();
 
   // Find the position in the above map that represents the shape of the
   // result:dim being inferred.
-  auto resultShapesSubMapPos = getResultsPositionInLoopsToShapeMap();
+  auto resultShapesSubMapPos = getResultsPositionInLoopsToShapeMap(*this);
 
   /// From loopsToShapesMap extract the submap that represents the shape of the
   /// (resultIdx, dim) needed.
   AffineMap loopToResultsShapeMap = loopsToShapesMap.getSliceMap(
       resultShapesSubMapPos.first,
       resultShapesSubMapPos.second - resultShapesSubMapPos.first);
   AffineMap resultShapesFromInputShapesMap =
       loopToResultsShapeMap.compose(getShapesToLoopsMap());