Skip to content

Commit 628976c

Browse files
cotacota
authored
Revert "[mlir] Make single value ValueRanges memory safer" (#123187)
Reverts #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<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>::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<void *, 3, llvm::pointer_union_detail::PointerUnionUIntTraits<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>>' 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<void *, 3, int, llvm::pointer_union_detail::PointerUnionUIntTraits<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>>::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<void *, 3, int, llvm::pointer_union_detail::PointerUnionUIntTraits<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>>::PointerIntPair' requested here 77 | : Base(ValTy(const_cast<void *>( | ^ llvm/llvm-project/mlir/include/mlir/IR/TypeRange.h:49:36: note: in instantiation of member function 'llvm::pointer_union_detail::PointerUnionMembers<llvm::PointerUnion<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>, llvm::PointerIntPair<void *, 3, int, llvm::pointer_union_detail::PointerUnionUIntTraits<const mlir::Value *, const mlir::Type *, mlir::OpOperand *, mlir::detail::OpResultImpl *, mlir::Type>>, 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. ```
1 parent c25bd6e commit 628976c

File tree

5 files changed

+16
-66
lines changed

5 files changed

+16
-66
lines changed

‎mlir/include/mlir/IR/TypeRange.h

+8-13
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,11 @@ namespace mlir {
2929
/// a SmallVector/std::vector. This class should be used in places that are not
3030
/// suitable for a more derived type (e.g. ArrayRef) or a template range
3131
/// parameter.
32-
class TypeRange
33-
: public llvm::detail::indexed_accessor_range_base<
34-
TypeRange,
35-
llvm::PointerUnion<const Value *, const Type *, OpOperand *,
36-
detail::OpResultImpl *, Type>,
37-
Type, Type, Type> {
32+
class TypeRange : public llvm::detail::indexed_accessor_range_base<
33+
TypeRange,
34+
llvm::PointerUnion<const Value *, const Type *,
35+
OpOperand *, detail::OpResultImpl *>,
36+
Type, Type, Type> {
3837
public:
3938
using RangeBaseT::RangeBaseT;
4039
TypeRange(ArrayRef<Type> types = std::nullopt);
@@ -45,11 +44,8 @@ class TypeRange
4544
TypeRange(ValueTypeRange<ValueRangeT> values)
4645
: TypeRange(ValueRange(ValueRangeT(values.begin().getCurrent(),
4746
values.end().getCurrent()))) {}
48-
49-
TypeRange(Type type) : TypeRange(type, /*count=*/1) {}
50-
template <typename Arg, typename = std::enable_if_t<
51-
std::is_constructible_v<ArrayRef<Type>, Arg> &&
52-
!std::is_constructible_v<Type, Arg>>>
47+
template <typename Arg, typename = std::enable_if_t<std::is_constructible<
48+
ArrayRef<Type>, Arg>::value>>
5349
TypeRange(Arg &&arg) : TypeRange(ArrayRef<Type>(std::forward<Arg>(arg))) {}
5450
TypeRange(std::initializer_list<Type> types)
5551
: TypeRange(ArrayRef<Type>(types)) {}
@@ -60,9 +56,8 @@ class TypeRange
6056
/// * A pointer to the first element of an array of types.
6157
/// * A pointer to the first element of an array of operands.
6258
/// * A pointer to the first element of an array of results.
63-
/// * A single 'Type' instance.
6459
using OwnerT = llvm::PointerUnion<const Value *, const Type *, OpOperand *,
65-
detail::OpResultImpl *, Type>;
60+
detail::OpResultImpl *>;
6661

6762
/// See `llvm::detail::indexed_accessor_range_base` for details.
6863
static OwnerT offset_base(OwnerT object, ptrdiff_t index);

‎mlir/include/mlir/IR/ValueRange.h

+8-8
Original file line numberDiff line numberDiff line change
@@ -374,16 +374,16 @@ class ResultRange::UseIterator final
374374
/// SmallVector/std::vector. This class should be used in places that are not
375375
/// suitable for a more derived type (e.g. ArrayRef) or a template range
376376
/// parameter.
377-
class ValueRange final : public llvm::detail::indexed_accessor_range_base<
378-
ValueRange,
379-
PointerUnion<const Value *, OpOperand *,
380-
detail::OpResultImpl *, Value>,
381-
Value, Value, Value> {
377+
class ValueRange final
378+
: public llvm::detail::indexed_accessor_range_base<
379+
ValueRange,
380+
PointerUnion<const Value *, OpOperand *, detail::OpResultImpl *>,
381+
Value, Value, Value> {
382382
public:
383383
/// The type representing the owner of a ValueRange. This is either a list of
384-
/// values, operands, or results or a single value.
384+
/// values, operands, or results.
385385
using OwnerT =
386-
PointerUnion<const Value *, OpOperand *, detail::OpResultImpl *, Value>;
386+
PointerUnion<const Value *, OpOperand *, detail::OpResultImpl *>;
387387

388388
using RangeBaseT::RangeBaseT;
389389

@@ -392,7 +392,7 @@ class ValueRange final : public llvm::detail::indexed_accessor_range_base<
392392
std::is_constructible<ArrayRef<Value>, Arg>::value &&
393393
!std::is_convertible<Arg, Value>::value>>
394394
ValueRange(Arg &&arg) : ValueRange(ArrayRef<Value>(std::forward<Arg>(arg))) {}
395-
ValueRange(Value value) : ValueRange(value, /*count=*/1) {}
395+
ValueRange(const Value &value) : ValueRange(&value, /*count=*/1) {}
396396
ValueRange(const std::initializer_list<Value> &values)
397397
: ValueRange(ArrayRef<Value>(values)) {}
398398
ValueRange(iterator_range<OperandRange::iterator> values)

‎mlir/lib/IR/OperationSupport.cpp

-13
Original file line numberDiff line numberDiff line change
@@ -653,15 +653,6 @@ ValueRange::ValueRange(ResultRange values)
653653
/// See `llvm::detail::indexed_accessor_range_base` for details.
654654
ValueRange::OwnerT ValueRange::offset_base(const OwnerT &owner,
655655
ptrdiff_t index) {
656-
if (llvm::isa_and_nonnull<Value>(owner)) {
657-
// Prevent out-of-bounds indexing for single values.
658-
// Note that we do allow an index of 1 as is required by 'slice'ing that
659-
// returns an empty range. This also matches the usual rules of C++ of being
660-
// allowed to index past the last element of an array.
661-
assert(index <= 1 && "out-of-bound offset into single-value 'ValueRange'");
662-
// Return nullptr to quickly cause segmentation faults on misuse.
663-
return index == 0 ? owner : nullptr;
664-
}
665656
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(owner))
666657
return {value + index};
667658
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))
@@ -670,10 +661,6 @@ ValueRange::OwnerT ValueRange::offset_base(const OwnerT &owner,
670661
}
671662
/// See `llvm::detail::indexed_accessor_range_base` for details.
672663
Value ValueRange::dereference_iterator(const OwnerT &owner, ptrdiff_t index) {
673-
if (auto value = llvm::dyn_cast_if_present<Value>(owner)) {
674-
assert(index == 0 && "cannot offset into single-value 'ValueRange'");
675-
return value;
676-
}
677664
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(owner))
678665
return value[index];
679666
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))

‎mlir/lib/IR/TypeRange.cpp

-15
Original file line numberDiff line numberDiff line change
@@ -31,23 +31,12 @@ TypeRange::TypeRange(ValueRange values) : TypeRange(OwnerT(), values.size()) {
3131
this->base = result;
3232
else if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))
3333
this->base = operand;
34-
else if (auto value = llvm::dyn_cast_if_present<Value>(owner))
35-
this->base = value.getType();
3634
else
3735
this->base = cast<const Value *>(owner);
3836
}
3937

4038
/// See `llvm::detail::indexed_accessor_range_base` for details.
4139
TypeRange::OwnerT TypeRange::offset_base(OwnerT object, ptrdiff_t index) {
42-
if (llvm::isa_and_nonnull<Type>(object)) {
43-
// Prevent out-of-bounds indexing for single values.
44-
// Note that we do allow an index of 1 as is required by 'slice'ing that
45-
// returns an empty range. This also matches the usual rules of C++ of being
46-
// allowed to index past the last element of an array.
47-
assert(index <= 1 && "out-of-bound offset into single-value 'ValueRange'");
48-
// Return nullptr to quickly cause segmentation faults on misuse.
49-
return index == 0 ? object : nullptr;
50-
}
5140
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(object))
5241
return {value + index};
5342
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(object))
@@ -59,10 +48,6 @@ TypeRange::OwnerT TypeRange::offset_base(OwnerT object, ptrdiff_t index) {
5948

6049
/// See `llvm::detail::indexed_accessor_range_base` for details.
6150
Type TypeRange::dereference_iterator(OwnerT object, ptrdiff_t index) {
62-
if (auto type = llvm::dyn_cast_if_present<Type>(object)) {
63-
assert(index == 0 && "cannot offset into single-value 'TypeRange'");
64-
return type;
65-
}
6651
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(object))
6752
return (value + index)->getType();
6853
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(object))

‎mlir/unittests/IR/OperationSupportTest.cpp

-17
Original file line numberDiff line numberDiff line change
@@ -313,21 +313,4 @@ TEST(OperationEquivalenceTest, HashWorksWithFlags) {
313313
op2->destroy();
314314
}
315315

316-
TEST(ValueRangeTest, ValueConstructable) {
317-
MLIRContext context;
318-
Builder builder(&context);
319-
320-
Operation *useOp =
321-
createOp(&context, /*operands=*/std::nullopt, builder.getIntegerType(16));
322-
// Valid construction despite a temporary 'OpResult'.
323-
ValueRange operands = useOp->getResult(0);
324-
325-
useOp->setOperands(operands);
326-
EXPECT_EQ(useOp->getNumOperands(), 1u);
327-
EXPECT_EQ(useOp->getOperand(0), useOp->getResult(0));
328-
329-
useOp->dropAllUses();
330-
useOp->destroy();
331-
}
332-
333316
} // namespace

0 commit comments

Comments
 (0)