[mlir][NFC] Remove TypeRange's constructors that cause ambiguity

[lipracer]

Summary

ArrayRef<Value> can implicit convert to ValueRange，when we call TypeRange(SmallVector<Value>) is ambiguity.
TypeRange(ValueRange values)
TypeRange(ArrayRef<Value> values)

[rriddle]

The windows failure looks real, can you take a look?

[lipracer]

windows msvc and gcc7 build error

[lipracer]

In D129475#3643666, @rriddle wrote:

The windows failure looks real, can you take a look?

done.

[rriddle]

In D129475#3646053, @lipracer wrote:

In D129475#3643666, @rriddle wrote:

The windows failure looks real, can you take a look?

done.

The debian failure looks real this time, or is that out-of-date?

[lipracer]

The msvc reported error is that: When we pass a parameter MutableArrayRef<BlockArgument> to the following function:

TypeRange(ValueTypeRange<ValueRangeT> values)
    : TypeRange(ValueRangeT(values.begin().getCurrent(),
                            values.end().getCurrent())) {}

then call delegating constructor:
TypeRange(ValueRange values); and TypeRange(const iterator_range<iterator> &range values) inherits from indexed_accessor_range_base
My second update trying to explicit call TypeRange(ValueRange values);
the clang reported error incomplete type 'mlir::ValueRange' and it defined in the OperationSupport.h file, and this file inclue<TypeRange.h>
I wonder if putting the ValueRange definition in a new file will work well.

[lipracer]

build fail

[lipracer]

format

[Mogball]

mlir/include/mlir/IR/TypeRange.h

50

I'm not really a fan of adding compiler-specific ifdefs

[rriddle]

mlir/include/mlir/IR/TypeRange.h

50

Yeah, we really shouldn't be adding these types of checks in regular code.

[lipracer]

mlir/include/mlir/IR/TypeRange.h

50

Is it reasonable to move the definition of ValueType from the operationSupport file to a new file?

[Mogball]

mlir/include/mlir/IR/TypeRange.h

50

ValueType? You mean TypeRange? Yes, I think it's reasonable to move it to a separate file TypeRange.h.

[lipracer]

mlir/include/mlir/IR/TypeRange.h

50

Sorry,I mean ValueRange.

[lipracer]

Split out ValueRange declare from OperationSupport.h

[Mogball]

Can you move the splitting of the files into another patch? It's kind of hard to review the diff otherwise

[rriddle]

mlir/include/mlir/IR/TypeRange.h

45–46

Why does doing this require moving ValueRange out of OperationSupport?

[lipracer]

mlir/include/mlir/IR/TypeRange.h

45–46

The prompt I get with the clang compiler is: ValueRange is imcomplete type,ValueRange is a forward declaration in the file TypeRange.h, and the file OperationSupport.h already includes the file TypeRange.h.The class TypeRange and the class ValueRange depend on each other.

[rriddle]

mlir/include/mlir/IR/TypeRange.h

45–46

Where do you get that error though?

[lipracer]

mlir/include/mlir/IR/TypeRange.h

45–46

When I change

template <typename ValueRangeT>
  TypeRange(ValueTypeRange<ValueRangeT> values) : TypeRange(ValueRangeT(values.begin().getCurrent(),
                              values.end().getCurrent())) {}

to

template <typename ValueRangeT>
TypeRange(ValueTypeRange<ValueRangeT> values)
    : TypeRange(ValueRange(ValueRangeT(values.begin().getCurrent(), values.end().getCurrent()))) {}

But if I don't change the MSVC compiler like this it will give an error.

[lipracer]

mlir/include/mlir/IR/TypeRange.h

45–46

My first change was to remove the following two redundant constructors:

explicit TypeRange(ArrayRef<Value> values);
explicit TypeRange(ArrayRef<BlockArgument> values)
    : TypeRange(ArrayRef<Value>(values.data(), values.size())) {}

[lipracer]

regression

[lipracer]

In D129475#3667404, @Mogball wrote:

Can you move the splitting of the files into another patch? It's kind of hard to review the diff otherwise

Split ValueRange definitions in this patch:
https://reviews.llvm.org/D130332

[Mogball]

Is the build status up to date?

[lipracer]

In D129475#3672668, @Mogball wrote:

Is the build status up to date?

This patch needs to be based on another patch: https://reviews.llvm.org/D130332.

[jpienaar]

In D129475#3673559, @lipracer wrote:

In D129475#3672668, @Mogball wrote:

Is the build status up to date?

This patch needs to be based on another patch: https://reviews.llvm.org/D130332.

The depends on comment is useful here (https://llvm.org/docs/Phabricator.html#using-patch-summaries), it shows up in the UI as "stack" too.

mlir/include/mlir/IR/TypeRange.h

class TypeRange : public llvm::detail::indexed_accessor_range_base<
                                          OpOperand *, detail::OpResultImpl *>,
                       Type, Type, Type> {
 public:
   using RangeBaseT::RangeBaseT;
   TypeRange(ArrayRef<Type> types = llvm::None);
   explicit TypeRange(OperandRange values);
   explicit TypeRange(ResultRange values);
   explicit TypeRange(ValueRange values);
-  explicit TypeRange(ArrayRef<Value> values);
-  explicit TypeRange(ArrayRef<BlockArgument> values)
-      : TypeRange(ArrayRef<Value>(values.data(), values.size())) {}
   template <typename ValueRangeT>
   TypeRange(ValueTypeRange<ValueRangeT> values)
-      : TypeRange(ValueRangeT(values.begin().getCurrent(),
-                              values.end().getCurrent())) {}
+      : TypeRange(ValueRange(ValueRangeT(values.begin().getCurrent(),
+                                         values.end().getCurrent()))) {}
   template <typename Arg,
             typename = typename std::enable_if_t<
                 std::is_constructible<ArrayRef<Type>, Arg>::value>>
   TypeRange(Arg &&arg) : TypeRange(ArrayRef<Type>(std::forward<Arg>(arg))) {}
   TypeRange(std::initializer_list<Type> types)
       : TypeRange(ArrayRef<Type>(types)) {}
 
 private:
   /// The owner of the range is either:
   /// * A pointer to the first element of an array of values.
   /// * A pointer to the first element of an array of types.
   /// * A pointer to the first element of an array of operands.

mlir/lib/IR/TypeRange.cpp

TypeRange::TypeRange(ArrayRef<Type> types)
     : TypeRange(types.data(), types.size()) {
   assert(llvm::all_of(types, [](Type t) { return t; }) &&
          "attempting to construct a TypeRange with null types");
 }
 TypeRange::TypeRange(OperandRange values)
     : TypeRange(values.begin().getBase(), values.size()) {}
 TypeRange::TypeRange(ResultRange values)
     : TypeRange(values.getBase(), values.size()) {}
-TypeRange::TypeRange(ArrayRef<Value> values)
-    : TypeRange(values.data(), values.size()) {}
 TypeRange::TypeRange(ValueRange values) : TypeRange(OwnerT(), values.size()) {
   if (count == 0)
     return;
   ValueRange::OwnerT owner = values.begin().getBase();
   if (auto *result = owner.dyn_cast<detail::OpResultImpl *>())
     this->base = result;
   else if (auto *operand = owner.dyn_cast<OpOperand *>())
     this->base = operand;