Remove obsolete `getAsmResultNames` from OpAsmDialectInterface

[mehdi_amini]

Summary

This is superseded by the same method on OpAsmOpInterface, which is
available on the Dialect through the Fallback mechanism,

[jpienaar]

I thought these served different purposes in that the latter provides a mechanism for doing some dialect wide (e.g., prefix with "foo_" unconditionally in dialect foo) like the regular dialect fallback hooks (query the op, query the dialect). Is this just unused? Or subsumed by way of being able to do this using dialect fallback hooks? Could you expand the summary?

[mehdi_amini]

In D117750#3257880, @jpienaar wrote:

I thought these served different purposes in that the latter provides a mechanism for doing some dialect wide (e.g., prefix with "foo_" unconditionally in dialect foo)

If this was the case, it shouldn't take the Operation as operand. I don't think Dialect interfaces should ever take operations, in particular since we support Dialect fallback for op interfaces.

Or subsumed by way of being able to do this using dialect fallback hooks? Could you expand the summary?

Added mention of the Fallback in the summary (I think that's what you meant).

mlir/include/mlir/IR/OpImplementation.h

public:
   /// end with a numeric digit([0-9]+).
   virtual AliasResult getAlias(Attribute attr, raw_ostream &os) const {
     return AliasResult::NoAlias;
   }
   virtual AliasResult getAlias(Type type, raw_ostream &os) const {
     return AliasResult::NoAlias;
   }
 
-  /// Get a special name to use when printing the given operation. See
-  /// OpAsmInterface.td#getAsmResultNames for usage details and documentation.
-  virtual void getAsmResultNames(Operation *op,
-                                 OpAsmSetValueNameFn setNameFn) const {}
 };
 } // namespace mlir
 
 //===--------------------------------------------------------------------===//
 // Operation OpAsm interface.
 //===--------------------------------------------------------------------===//
 
 /// The OpAsmOpInterface, see OpAsmInterface.td for more details.
 #include "mlir/IR/OpAsmInterface.h.inc"
 
 #endif

mlir/lib/IR/AsmPrinter.cpp

 
 namespace {
 /// This class manages the state of SSA value names.
 class SSANameState {
 public:
   /// A sentinel value used for values with names set.
   enum : unsigned { NameSentinel = ~0U };
 
-  SSANameState(Operation *op, const OpPrintingFlags &printerFlags,
-               DialectInterfaceCollection<OpAsmDialectInterface> &interfaces);
+  SSANameState(Operation *op, const OpPrintingFlags &printerFlags);
 
   /// Print the SSA identifier for the given value to 'stream'. If
   /// 'printResultNo' is true, it also presents the result number ('#' number)
   /// of this value.
   void printValueID(Value value, bool printResultNo, raw_ostream &stream) const;
 
   /// Return the result indices for each of the result groups registered by this
   /// operation, or empty if none exist.
private:
   /// This is the next ID to assign to a region entry block argument.
   unsigned nextArgumentID = 0;
   /// This is the next ID to assign when a name conflict is detected.
   unsigned nextConflictID = 0;
 
   /// These are the printing flags.  They control, eg., whether to print in
   /// generic form.
   OpPrintingFlags printerFlags;
-
-  DialectInterfaceCollection<OpAsmDialectInterface> &interfaces;
 };
 } // namespace
 
 SSANameState::SSANameState(
-    Operation *op, const OpPrintingFlags &printerFlags,
-    DialectInterfaceCollection<OpAsmDialectInterface> &interfaces)
-    : printerFlags(printerFlags), interfaces(interfaces) {
+    Operation *op, const OpPrintingFlags &printerFlags)
+    : printerFlags(printerFlags) {
   llvm::SaveAndRestore<unsigned> valueIDSaver(nextValueID);
   llvm::SaveAndRestore<unsigned> argumentIDSaver(nextArgumentID);
   llvm::SaveAndRestore<unsigned> conflictIDSaver(nextConflictID);
 
   // The naming context includes `nextValueID`, `nextArgumentID`,
   // `nextConflictID` and `usedNames` scoped HashTable. This information is
   // carried from the parent region.
   using UsedNamesScopeTy = llvm::ScopedHashTable<StringRef, char>::ScopeTy;
auto setResultNameFn = [&](Value result, StringRef name) {
 
     // Record the result number for groups not anchored at 0.
     if (int resultNo = result.cast<OpResult>().getResultNumber())
       resultGroups.push_back(resultNo);
   };
   if (!printerFlags.shouldPrintGenericOpForm()) {
     if (OpAsmOpInterface asmInterface = dyn_cast<OpAsmOpInterface>(&op))
       asmInterface.getAsmResultNames(setResultNameFn);
-    else if (auto *asmInterface = interfaces.getInterfaceFor(op.getDialect()))
-      asmInterface->getAsmResultNames(&op, setResultNameFn);
   }
 
   // If the first result wasn't numbered, give it a default number.
   if (valueIDs.try_emplace(resultBegin, nextValueID).second)
     ++nextValueID;
 
   // If this operation has multiple result groups, mark it.
   if (resultGroups.size() != 1) {

 //===----------------------------------------------------------------------===//
 
 namespace mlir {
 namespace detail {
 class AsmStateImpl {
 public:
   explicit AsmStateImpl(Operation *op, const OpPrintingFlags &printerFlags,
                         AsmState::LocationMap *locationMap)
-      : interfaces(op->getContext()), nameState(op, printerFlags, interfaces),
+      : interfaces(op->getContext()), nameState(op, printerFlags),
         printerFlags(printerFlags), locationMap(locationMap) {}
 
   /// Initialize the alias state to enable the printing of aliases.
   void initializeAliases(Operation *op) {
     aliasState.initialize(op, printerFlags, interfaces);
   }
 
   /// Get an instance of the OpAsmDialectInterface for the given dialect, or

mlir/test/IR/parser.mlir

 // CHECK-SAME: {"0 . 0", nested = {"0 . 0"}}
 func private @string_attr_name() attributes {"0 . 0", nested = {"0 . 0"}}
 
 // CHECK-LABEL: func private @nested_reference
 // CHECK: ref = @some_symbol::@some_nested_symbol
 func private @nested_reference() attributes {test.ref = @some_symbol::@some_nested_symbol }
 
 // CHECK-LABEL: func @custom_asm_names
-func @custom_asm_names() -> (i32, i32, i32, i32, i32, i32, i32) {
+func @custom_asm_names() -> (i32, i32, i32, i32, i32, i32) {
   // CHECK: %[[FIRST:first.*]], %[[MIDDLE:middle_results.*]]:2, %[[LAST:[0-9]+]]
   %0, %1:2, %2 = "test.asm_interface_op"() : () -> (i32, i32, i32, i32)
 
   // CHECK: %[[FIRST_2:first.*]], %[[LAST_2:[0-9]+]]
   %3, %4 = "test.asm_interface_op"() : () -> (i32, i32)
 
-  // CHECK: %[[RESULT:result.*]]
-  %5 = "test.asm_dialect_interface_op"() : () -> (i32)
-
   // CHECK: return %[[FIRST]], %[[MIDDLE]]#0, %[[MIDDLE]]#1, %[[LAST]], %[[FIRST_2]], %[[LAST_2]]
-  return %0, %1#0, %1#1, %2, %3, %4, %5 : i32, i32, i32, i32, i32, i32, i32
+  return %0, %1#0, %1#1, %2, %3, %4 : i32, i32, i32, i32, i32, i32
 }
 
 
 // CHECK-LABEL: func @pretty_names
 
 // This tests the behavior
 func @pretty_names() {
   // Simple case, should parse and print as %x being an implied 'name'

mlir/test/lib/Dialect/Test/TestDialect.cpp

if (auto intType = type.dyn_cast<TestIntegerType>()) {
               TestIntegerType::SignednessSemantics::Unsigned &&
           intType.getWidth() == 8) {
         os << "test_ui8";
         return AliasResult::FinalAlias;
       }
     }
     return AliasResult::NoAlias;
   }
-
-  void getAsmResultNames(Operation *op,
-                         OpAsmSetValueNameFn setNameFn) const final {
-    if (auto asmOp = dyn_cast<AsmDialectInterfaceOp>(op))
-      setNameFn(asmOp, "result");
-  }
 };
 
 struct TestDialectFoldInterface : public DialectFoldInterface {
   using DialectFoldInterface::DialectFoldInterface;
 
   /// Registered hook to check if the given region, which is attached to an
   /// operation that is *not* isolated from above, should be used when
   /// materializing constants.