From 628976c8345e235d4f71a0715f1990ad8b5bbcf7 Mon Sep 17 00:00:00 2001 From: Emilio Cota Date: Thu, 16 Jan 2025 11:33:13 +0000 Subject: [PATCH] Revert "[mlir] Make single value `ValueRange`s memory safer" (#123187) Reverts llvm/llvm-project#121996 because it broke an emscripten build with `--target=wasm32-unknown-emscripten`: ``` llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:172:17: error: static assertion failed due to requirement '3U <= PointerUnionUIntTraits::NumLowBitsAvailable': PointerIntPair with integer size too large for pointer 172 | static_assert(IntBits <= PtrTraits::NumLowBitsAvailable, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:111:13: note: in instantiation of template class 'llvm::PointerIntPairInfo>' requested here 111 | Value = Info::updateInt(Info::updatePointer(0, PtrVal), | ^ llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:89:5: note: in instantiation of member function 'llvm::PointerIntPair>::setPointerAndInt' requested here 89 | setPointerAndInt(PtrVal, IntVal); | ^ llvm/llvm-project/llvm/include/llvm/ADT/PointerUnion.h:77:16: note: in instantiation of member function 'llvm::PointerIntPair>::PointerIntPair' requested here 77 | : Base(ValTy(const_cast( | ^ llvm/llvm-project/mlir/include/mlir/IR/TypeRange.h:49:36: note: in instantiation of member function 'llvm::pointer_union_detail::PointerUnionMembers, llvm::PointerIntPair>, 4, mlir::Type>::PointerUnionMembers' requested here 49 | TypeRange(Type type) : TypeRange(type, /*count=*/1) {} | ^ llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:172:25: note: expression evaluates to '3 <= 2' 172 | static_assert(IntBits <= PtrTraits::NumLowBitsAvailable, | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. ``` --- mlir/include/mlir/IR/TypeRange.h | 21 ++++++++------------- mlir/include/mlir/IR/ValueRange.h | 16 ++++++++-------- mlir/lib/IR/OperationSupport.cpp | 13 ------------- mlir/lib/IR/TypeRange.cpp | 15 --------------- mlir/unittests/IR/OperationSupportTest.cpp | 17 ----------------- 5 files changed, 16 insertions(+), 66 deletions(-) diff --git a/mlir/include/mlir/IR/TypeRange.h b/mlir/include/mlir/IR/TypeRange.h index fa63435b188e98..99fabab334f922 100644 --- a/mlir/include/mlir/IR/TypeRange.h +++ b/mlir/include/mlir/IR/TypeRange.h @@ -29,12 +29,11 @@ namespace mlir { /// a SmallVector/std::vector. This class should be used in places that are not /// suitable for a more derived type (e.g. ArrayRef) or a template range /// parameter. -class TypeRange - : public llvm::detail::indexed_accessor_range_base< - TypeRange, - llvm::PointerUnion, - Type, Type, Type> { +class TypeRange : public llvm::detail::indexed_accessor_range_base< + TypeRange, + llvm::PointerUnion, + Type, Type, Type> { public: using RangeBaseT::RangeBaseT; TypeRange(ArrayRef types = std::nullopt); @@ -45,11 +44,8 @@ class TypeRange TypeRange(ValueTypeRange values) : TypeRange(ValueRange(ValueRangeT(values.begin().getCurrent(), values.end().getCurrent()))) {} - - TypeRange(Type type) : TypeRange(type, /*count=*/1) {} - template , Arg> && - !std::is_constructible_v>> + template , Arg>::value>> TypeRange(Arg &&arg) : TypeRange(ArrayRef(std::forward(arg))) {} TypeRange(std::initializer_list types) : TypeRange(ArrayRef(types)) {} @@ -60,9 +56,8 @@ class TypeRange /// * A pointer to the first element of an array of types. /// * A pointer to the first element of an array of operands. /// * A pointer to the first element of an array of results. - /// * A single 'Type' instance. using OwnerT = llvm::PointerUnion; + detail::OpResultImpl *>; /// See `llvm::detail::indexed_accessor_range_base` for details. static OwnerT offset_base(OwnerT object, ptrdiff_t index); diff --git a/mlir/include/mlir/IR/ValueRange.h b/mlir/include/mlir/IR/ValueRange.h index d5b067a79200d8..4b421c08d8418e 100644 --- a/mlir/include/mlir/IR/ValueRange.h +++ b/mlir/include/mlir/IR/ValueRange.h @@ -374,16 +374,16 @@ class ResultRange::UseIterator final /// SmallVector/std::vector. This class should be used in places that are not /// suitable for a more derived type (e.g. ArrayRef) or a template range /// parameter. -class ValueRange final : public llvm::detail::indexed_accessor_range_base< - ValueRange, - PointerUnion, - Value, Value, Value> { +class ValueRange final + : public llvm::detail::indexed_accessor_range_base< + ValueRange, + PointerUnion, + Value, Value, Value> { public: /// The type representing the owner of a ValueRange. This is either a list of - /// values, operands, or results or a single value. + /// values, operands, or results. using OwnerT = - PointerUnion; + PointerUnion; using RangeBaseT::RangeBaseT; @@ -392,7 +392,7 @@ class ValueRange final : public llvm::detail::indexed_accessor_range_base< std::is_constructible, Arg>::value && !std::is_convertible::value>> ValueRange(Arg &&arg) : ValueRange(ArrayRef(std::forward(arg))) {} - ValueRange(Value value) : ValueRange(value, /*count=*/1) {} + ValueRange(const Value &value) : ValueRange(&value, /*count=*/1) {} ValueRange(const std::initializer_list &values) : ValueRange(ArrayRef(values)) {} ValueRange(iterator_range values) diff --git a/mlir/lib/IR/OperationSupport.cpp b/mlir/lib/IR/OperationSupport.cpp index 803fcd8d18fbd5..957195202d78d2 100644 --- a/mlir/lib/IR/OperationSupport.cpp +++ b/mlir/lib/IR/OperationSupport.cpp @@ -653,15 +653,6 @@ ValueRange::ValueRange(ResultRange values) /// See `llvm::detail::indexed_accessor_range_base` for details. ValueRange::OwnerT ValueRange::offset_base(const OwnerT &owner, ptrdiff_t index) { - if (llvm::isa_and_nonnull(owner)) { - // Prevent out-of-bounds indexing for single values. - // Note that we do allow an index of 1 as is required by 'slice'ing that - // returns an empty range. This also matches the usual rules of C++ of being - // allowed to index past the last element of an array. - assert(index <= 1 && "out-of-bound offset into single-value 'ValueRange'"); - // Return nullptr to quickly cause segmentation faults on misuse. - return index == 0 ? owner : nullptr; - } if (const auto *value = llvm::dyn_cast_if_present(owner)) return {value + index}; if (auto *operand = llvm::dyn_cast_if_present(owner)) @@ -670,10 +661,6 @@ ValueRange::OwnerT ValueRange::offset_base(const OwnerT &owner, } /// See `llvm::detail::indexed_accessor_range_base` for details. Value ValueRange::dereference_iterator(const OwnerT &owner, ptrdiff_t index) { - if (auto value = llvm::dyn_cast_if_present(owner)) { - assert(index == 0 && "cannot offset into single-value 'ValueRange'"); - return value; - } if (const auto *value = llvm::dyn_cast_if_present(owner)) return value[index]; if (auto *operand = llvm::dyn_cast_if_present(owner)) diff --git a/mlir/lib/IR/TypeRange.cpp b/mlir/lib/IR/TypeRange.cpp index 7e5f99c884512e..f8878303727d4f 100644 --- a/mlir/lib/IR/TypeRange.cpp +++ b/mlir/lib/IR/TypeRange.cpp @@ -31,23 +31,12 @@ TypeRange::TypeRange(ValueRange values) : TypeRange(OwnerT(), values.size()) { this->base = result; else if (auto *operand = llvm::dyn_cast_if_present(owner)) this->base = operand; - else if (auto value = llvm::dyn_cast_if_present(owner)) - this->base = value.getType(); else this->base = cast(owner); } /// See `llvm::detail::indexed_accessor_range_base` for details. TypeRange::OwnerT TypeRange::offset_base(OwnerT object, ptrdiff_t index) { - if (llvm::isa_and_nonnull(object)) { - // Prevent out-of-bounds indexing for single values. - // Note that we do allow an index of 1 as is required by 'slice'ing that - // returns an empty range. This also matches the usual rules of C++ of being - // allowed to index past the last element of an array. - assert(index <= 1 && "out-of-bound offset into single-value 'ValueRange'"); - // Return nullptr to quickly cause segmentation faults on misuse. - return index == 0 ? object : nullptr; - } if (const auto *value = llvm::dyn_cast_if_present(object)) return {value + index}; if (auto *operand = llvm::dyn_cast_if_present(object)) @@ -59,10 +48,6 @@ TypeRange::OwnerT TypeRange::offset_base(OwnerT object, ptrdiff_t index) { /// See `llvm::detail::indexed_accessor_range_base` for details. Type TypeRange::dereference_iterator(OwnerT object, ptrdiff_t index) { - if (auto type = llvm::dyn_cast_if_present(object)) { - assert(index == 0 && "cannot offset into single-value 'TypeRange'"); - return type; - } if (const auto *value = llvm::dyn_cast_if_present(object)) return (value + index)->getType(); if (auto *operand = llvm::dyn_cast_if_present(object)) diff --git a/mlir/unittests/IR/OperationSupportTest.cpp b/mlir/unittests/IR/OperationSupportTest.cpp index 2a1b8d2ef7f55b..f94dc784458077 100644 --- a/mlir/unittests/IR/OperationSupportTest.cpp +++ b/mlir/unittests/IR/OperationSupportTest.cpp @@ -313,21 +313,4 @@ TEST(OperationEquivalenceTest, HashWorksWithFlags) { op2->destroy(); } -TEST(ValueRangeTest, ValueConstructable) { - MLIRContext context; - Builder builder(&context); - - Operation *useOp = - createOp(&context, /*operands=*/std::nullopt, builder.getIntegerType(16)); - // Valid construction despite a temporary 'OpResult'. - ValueRange operands = useOp->getResult(0); - - useOp->setOperands(operands); - EXPECT_EQ(useOp->getNumOperands(), 1u); - EXPECT_EQ(useOp->getOperand(0), useOp->getResult(0)); - - useOp->dropAllUses(); - useOp->destroy(); -} - } // namespace