[mlir][core] Fix ValueRange printing in AsmPrinter

[wpmed92]

Summary

The ValueRange printing behaviour of OpAsmPrinter and AsmPrinter is different, as reported 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;
}

[wpmed92]

@rriddle Can you please commit it on my behalf? This is my first patch so I don't have commit access yet.

[rriddle]

Sure, can you provide an author name and email to attach to the commit? (arc doesn't want to patch this commit automatically for me).

[wpmed92]

Sure, author name: wpmed92 email: [e-mail]
Thank you!

[rkayaith]

since ValueRange is implicitly converted to TypeRange

This seems like the core issue to me, this doesn't seem like something that should happen implicitly. And the conversion is already supposed to be explicit: https://github.com/llvm/llvm-project/blob/5ac69674bf4fbe4adaca4170a2ad60c8a32613ed/mlir/include/mlir/IR/TypeRange.h#L42

[lattner]

Cool didn't know about enable_if = delete, TIL thank you for tackling this!

[wpmed92]

@rkayaith How about L43, where it takes a ValueTypeRange<ValueRange>? ValueRange to ValueTypeRange<ValueRange> can be implicit.

[wpmed92]

Thank you @lattner, happy to be working on MLIR. I hope I can contribute more.

[rkayaith]

In D144833#4156592, @wpmed92 wrote:

@rkayaith How about L43, where it takes a ValueTypeRange<ValueRange>? ValueRange to ValueTypeRange<ValueRange> can be implicit.

Can it be made explicit/hidden/disabled? The intended usage seems to be that ValueTypeRanges are created explicitly through {ValueRange,ResultRange,etc}::getTypes()

[Mogball]

It looks like this was already committed somehow. I'm seeing the changes in main.

mlir/include/mlir/IR/OpImplementation.h

 
 template <typename AsmPrinterT, typename ValueRangeT>
 inline std::enable_if_t<std::is_base_of<AsmPrinter, AsmPrinterT>::value,
                         AsmPrinterT &>
 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 &>
 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 &>
 operator<<(AsmPrinterT &p, ArrayRef<ElementT> types) {
   llvm::interleaveComma(types, p);
   return p;
 }