[mlir] Fix indexed_accessor_range to properly forward the derived class.

[rriddle]

Summary

This fixes the return value of helper methods on the base range class.

[merge_guards_bot]

Unit tests: pass. 61173 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[jpienaar]

Could we add a test that this would have hit? (I'm thinking of one just assigning the sliced/drop_front one).

[rriddle]

Add test file.

[merge_guards_bot]

Unit tests: pass. 61236 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

mlir/include/mlir/IR/OperationSupport.h

 //===- OperationSupport.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines a number of support types that Operation and related
 // classes build on top of.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_IR_OPERATION_SUPPORT_H
 #define MLIR_IR_OPERATION_SUPPORT_H
 
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/Identifier.h"
 #include "mlir/IR/Location.h"
 #include "mlir/IR/Types.h"
 #include "mlir/IR/Value.h"
 #include "mlir/Support/LogicalResult.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/Support/TrailingObjects.h"
 #include <memory>
using indexed_accessor_range<ResultRange, Operation *, OpResult, OpResult,
                                OpResult>::indexed_accessor_range;
   ResultRange(Operation *op);
 
   /// Returns the types of the values within this range.
   using type_iterator = ValueTypeIterator<iterator>;
   iterator_range<type_iterator> getTypes() const { return {begin(), end()}; }
 
 private:
-  /// See `detail::indexed_accessor_range_base` for details.
-  static OpResult dereference_iterator(Operation *op, ptrdiff_t index);
+  /// See `indexed_accessor_range` for details.
+  static OpResult dereference(Operation *op, ptrdiff_t index);
 
   /// Allow access to `dereference_iterator`.
   friend indexed_accessor_range<ResultRange, Operation *, OpResult, OpResult,
                                 OpResult>;
 };
 
 //===----------------------------------------------------------------------===//
 // ValueRange

mlir/include/mlir/Support/STLExtras.h

 //===- STLExtras.h - STL-like extensions that are used by MLIR --*- C++ -*-===//
 //
 // Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains stuff that should be arguably sunk down to the LLVM
 // Support/STLExtras.h file over time.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_SUPPORT_STLEXTRAS_H
 #define MLIR_SUPPORT_STLEXTRAS_H
 
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/STLExtras.h"
 
 namespace mlir {
 
 namespace detail {
 template <typename RangeT>
 using ValueOfRange = typename std::remove_reference<decltype(
     *std::begin(std::declval<RangeT &>()))>::type;
friend indexed_accessor_range_base<DerivedT, BaseT, T, PointerT,
                                        ReferenceT>;
   };
 
   indexed_accessor_range_base(iterator begin, iterator end)
       : base(DerivedT::offset_base(begin.getBase(), begin.getIndex())),
         count(end.getIndex() - begin.getIndex()) {}
   indexed_accessor_range_base(const iterator_range<iterator> &range)
       : indexed_accessor_range_base(range.begin(), range.end()) {}
+  indexed_accessor_range_base(BaseT base, ptrdiff_t count)
+      : base(base), count(count) {}
 
   iterator begin() const { return iterator(base, 0); }
   iterator end() const { return iterator(base, count); }
   ReferenceT operator[](unsigned index) const {
     assert(index < size() && "invalid index for value range");
     return DerivedT::dereference_iterator(base, index);
   }
 
public:
   /// Allow conversion to SmallVector if necessary.
   /// TODO(riverriddle) Remove this when SmallVector accepts different range
   /// types in its constructor.
   template <typename SVT, unsigned N> operator SmallVector<SVT, N>() const {
     return {begin(), end()};
   }
 
 protected:
-  indexed_accessor_range_base(BaseT base, ptrdiff_t count)
-      : base(base), count(count) {}
   indexed_accessor_range_base(const indexed_accessor_range_base &) = default;
   indexed_accessor_range_base(indexed_accessor_range_base &&) = default;
   indexed_accessor_range_base &
   operator=(const indexed_accessor_range_base &) = default;
 
   /// The base that owns the provided range of values.
   BaseT base;
   /// The size from the owning range.
   ptrdiff_t count;
 };
 } // end namespace detail
 
 /// This class provides an implementation of a range of
 /// indexed_accessor_iterators where the base is not indexable. Ranges with
 /// bases that are offsetable should derive from indexed_accessor_range_base
 /// instead. Derived range classes are expected to implement the following
 /// static method:
-///   * ReferenceT dereference_iterator(const BaseT &base, ptrdiff_t index)
+///   * ReferenceT dereference(const BaseT &base, ptrdiff_t index)
 ///     - Derefence an iterator pointing to a parent base at the given index.
 template <typename DerivedT, typename BaseT, typename T,
           typename PointerT = T *, typename ReferenceT = T &>
 class indexed_accessor_range
     : public detail::indexed_accessor_range_base<
-          indexed_accessor_range<DerivedT, BaseT, T, PointerT, ReferenceT>,
-          std::pair<BaseT, ptrdiff_t>, T, PointerT, ReferenceT> {
+          DerivedT, std::pair<BaseT, ptrdiff_t>, T, PointerT, ReferenceT> {
 public:
+  indexed_accessor_range(BaseT base, ptrdiff_t startIndex, ptrdiff_t count)
+      : detail::indexed_accessor_range_base<
+            DerivedT, std::pair<BaseT, ptrdiff_t>, T, PointerT, ReferenceT>(
+            std::make_pair(base, startIndex), count) {}
   using detail::indexed_accessor_range_base<
-      indexed_accessor_range<DerivedT, BaseT, T, PointerT, ReferenceT>,
-      std::pair<BaseT, ptrdiff_t>, T, PointerT,
+      DerivedT, std::pair<BaseT, ptrdiff_t>, T, PointerT,
       ReferenceT>::indexed_accessor_range_base;
 
   /// Returns the current base of the range.
   const BaseT &getBase() const { return this->base.first; }
 
   /// Returns the current start index of the range.
   ptrdiff_t getStartIndex() const { return this->base.second; }
 
-protected:
-  indexed_accessor_range(BaseT base, ptrdiff_t startIndex, ptrdiff_t count)
-      : detail::indexed_accessor_range_base<
-            indexed_accessor_range<DerivedT, BaseT, T, PointerT, ReferenceT>,
-            std::pair<BaseT, ptrdiff_t>, T, PointerT, ReferenceT>(
-            std::make_pair(base, startIndex), count) {}
-
-private:
   /// See `detail::indexed_accessor_range_base` for details.
   static std::pair<BaseT, ptrdiff_t>
   offset_base(const std::pair<BaseT, ptrdiff_t> &base, ptrdiff_t index) {
     // We encode the internal base as a pair of the derived base and a start
     // index into the derived base.
     return std::make_pair(base.first, base.second + index);
   }
   /// See `detail::indexed_accessor_range_base` for details.
   static ReferenceT
   dereference_iterator(const std::pair<BaseT, ptrdiff_t> &base,
                        ptrdiff_t index) {
-    return DerivedT::dereference_iterator(base.first, base.second + index);
+    return DerivedT::dereference(base.first, base.second + index);
   }
-
-  /// Allow access to `offset_base` and `dereference_iterator`.
-  friend detail::indexed_accessor_range_base<
-      indexed_accessor_range<DerivedT, BaseT, T, PointerT, ReferenceT>,
-      std::pair<BaseT, ptrdiff_t>, T, PointerT, ReferenceT>;
 };
 
 /// Given a container of pairs, return a range over the second elements.
 template <typename ContainerTy> auto make_second_range(ContainerTy &&c) {
   return llvm::map_range(
       std::forward<ContainerTy>(c),
       [](decltype((*std::begin(c))) elt) -> decltype((elt.second)) {
         return elt.second;

mlir/lib/IR/OperationSupport.cpp

 //===- OperationSupport.cpp -----------------------------------------------===//
 //
 // Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains out-of-line implementations of the support types that
 // Operation and related classes build on top of.
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/IR/OperationSupport.h"
 #include "mlir/IR/Block.h"
 #include "mlir/IR/Operation.h"
 using namespace mlir;
 
 //===----------------------------------------------------------------------===//
 // OperationState
 //===----------------------------------------------------------------------===//
 
OperandRange::OperandRange(Operation *op)
     : OperandRange(op->getOpOperands().data(), op->getNumOperands()) {}
 
 //===----------------------------------------------------------------------===//
 // ResultRange
 
 ResultRange::ResultRange(Operation *op)
     : ResultRange(op, /*startIndex=*/0, op->getNumResults()) {}
 
-/// See `detail::indexed_accessor_range_base` for details.
-OpResult ResultRange::dereference_iterator(Operation *op, ptrdiff_t index) {
+/// See `indexed_accessor_range` for details.
+OpResult ResultRange::dereference(Operation *op, ptrdiff_t index) {
   return op->getResult(index);
 }
 
 //===----------------------------------------------------------------------===//
 // ValueRange
 
 ValueRange::ValueRange(ArrayRef<Value> values)
     : ValueRange(values.data(), values.size()) {}

mlir/unittests/CMakeLists.txt

 add_custom_target(MLIRUnitTests)
 set_target_properties(MLIRUnitTests PROPERTIES FOLDER "MLIR Tests")
 
 function(add_mlir_unittest test_dirname)
   add_unittest(MLIRUnitTests ${test_dirname} ${ARGN})
 endfunction()
 
 add_subdirectory(ADT)
 add_subdirectory(Dialect)
 add_subdirectory(IR)
 add_subdirectory(Pass)
 add_subdirectory(SDBM)
+add_subdirectory(Support)
 add_subdirectory(TableGen)

mlir/unittests/Support/CMakeLists.txt

+add_mlir_unittest(MLIRSupportTests
+  IndexedAccessorTest.cpp
+)
+
+target_link_libraries(MLIRSupportTests
+  PRIVATE MLIRSupport)

mlir/unittests/Support/IndexedAccessorTest.cpp

+//===- IndexedAccessorTest.cpp - Indexed Accessor Tests -------------------===//
+//
+// Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Support/STLExtras.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "gmock/gmock.h"
+
+using namespace mlir;
+using namespace mlir::detail;
+
+namespace {
+/// Simple indexed accessor range that wraps an array.
+template <typename T>
+struct ArrayIndexedAccessorRange
+    : public indexed_accessor_range<ArrayIndexedAccessorRange<T>, T *, T> {
+  ArrayIndexedAccessorRange(T *data, ptrdiff_t start, ptrdiff_t numElements)
+      : indexed_accessor_range<ArrayIndexedAccessorRange<T>, T *, T>(
+            data, start, numElements) {}
+  using indexed_accessor_range<ArrayIndexedAccessorRange<T>, T *,
+                               T>::indexed_accessor_range;
+
+  /// See `indexed_accessor_range` for details.
+  static T &dereference(T *data, ptrdiff_t index) { return data[index]; }
+};
+} // end anonymous namespace
+
+template <typename T>
+static void compareData(ArrayIndexedAccessorRange<T> range,
+                        ArrayRef<T> referenceData) {
+  ASSERT_TRUE(referenceData.size() == range.size());
+  ASSERT_TRUE(std::equal(range.begin(), range.end(), referenceData.begin()));
+}
+
+namespace {
+TEST(AccessorRange, SliceTest) {
+  int rawData[] = {0, 1, 2, 3, 4};
+  ArrayRef<int> data = llvm::makeArrayRef(rawData);
+
+  ArrayIndexedAccessorRange<int> range(rawData, /*start=*/0, /*numElements=*/5);
+  compareData(range, data);
+  compareData(range.slice(2, 3), data.slice(2, 3));
+  compareData(range.slice(0, 5), data.slice(0, 5));
+}
+} // end anonymous namespace