[Mlir-commits] [mlir] 964997a - [mlir][core] Fix ValueRange printing in AsmPrinter

[River Riddle]

Author: wpmed92
Date: 2023-02-27T14:53:59-08:00
New Revision: 964997a143cfa0356d1cd21eb3e6d2170c975c38

URL: https://github.com/llvm/llvm-project/commit/964997a143cfa0356d1cd21eb3e6d2170c975c38
DIFF: https://github.com/llvm/llvm-project/commit/964997a143cfa0356d1cd21eb3e6d2170c975c38.diff

LOG: [mlir][core] Fix ValueRange printing in AsmPrinter

The ValueRange printing behaviour of `OpAsmPrinter` and `AsmPrinter` is different, as reported [[ https://github.com/llvm/llvm-project/issues/59334 | here ]]

```
static void testPrint(AsmPrinter &p, Operation *op, ValueRange operands) {
  p << '(' << operands << ')';
}
```
Although the base `AsmPrinter` is passed as the first parameter (and not `OpAsmPrinter`), the code compiles fine. However, instead of the SSA values, the types for the operands will be printed. This is a violation of the Liskov Substitution Principle.
The desired behaviour would be that the above code does not compile. The reason it compiles, is that for the above code, the `TypeRange` version will be selected for the `<<` operator, since `ValueRange` is implicitly converted to `TypeRange`:
```
template <typename AsmPrinterT>
inline std::enable_if_t<std::is_base_of<AsmPrinter, AsmPrinterT>::value,
                        AsmPrinterT &>
operator<<(AsmPrinterT &p, const TypeRange &types) {
  llvm::interleaveComma(types, p);
  return p;
}
```

Added: 
    

Modified: 
    mlir/include/mlir/IR/OpImplementation.h

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index 92f4c610a9031..e2cd8dc358e72 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -302,6 +302,7 @@ operator<<(AsmPrinterT &p, const ValueTypeRange<ValueRangeT> &types) {
   llvm::interleaveComma(types, p);
   return p;
 }
+
 template <typename AsmPrinterT>
 inline std::enable_if_t<std::is_base_of<AsmPrinter, AsmPrinterT>::value,
                         AsmPrinterT &>
@@ -309,6 +310,18 @@ operator<<(AsmPrinterT &p, const TypeRange &types) {
   llvm::interleaveComma(types, p);
   return p;
 }
+
+// Prevent matching the TypeRange version above for ValueRange
+// printing through base AsmPrinter. This is needed so that the
+// ValueRange printing behaviour does not change from printing
+// the SSA values to printing the types for the operands when
+// using AsmPrinter instead of OpAsmPrinter.
+template <typename AsmPrinterT, typename T>
+inline std::enable_if_t<std::is_same<AsmPrinter, AsmPrinterT>::value &&
+                            std::is_convertible<T &, ValueRange>::value,
+                        AsmPrinterT &>
+operator<<(AsmPrinterT &p, const T &other) = delete;
+
 template <typename AsmPrinterT, typename ElementT>
 inline std::enable_if_t<std::is_base_of<AsmPrinter, AsmPrinterT>::value,
                         AsmPrinterT &>