Skip to content

Commit

Permalink
Revert "[mlir] Make single value ValueRanges memory safer" (llvm#12…
Browse files Browse the repository at this point in the history 
…3187)

Reverts llvm#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.
```
  • Loading branch information
@cota
cota authored Jan 16, 2025
1 parent c25bd6e commit 628976c
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 66 deletions.
21 changes: 8 additions & 13 deletions mlir/include/mlir/IR/TypeRange.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const Value *, const Type *, OpOperand *,
detail::OpResultImpl *, Type>,
Type, Type, Type> {
class TypeRange : public llvm::detail::indexed_accessor_range_base<
TypeRange,
llvm::PointerUnion<const Value *, const Type *,
OpOperand *, detail::OpResultImpl *>,
Type, Type, Type> {
public:
using RangeBaseT::RangeBaseT;
TypeRange(ArrayRef<Type> types = std::nullopt);
Expand All @@ -45,11 +44,8 @@ class TypeRange
TypeRange(ValueTypeRange<ValueRangeT> values)
: TypeRange(ValueRange(ValueRangeT(values.begin().getCurrent(),
values.end().getCurrent()))) {}

TypeRange(Type type) : TypeRange(type, /*count=*/1) {}
template <typename Arg, typename = std::enable_if_t<
std::is_constructible_v<ArrayRef<Type>, Arg> &&
!std::is_constructible_v<Type, Arg>>>
template <typename Arg, 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)) {}
Expand All @@ -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<const Value *, const Type *, OpOperand *,
detail::OpResultImpl *, Type>;
detail::OpResultImpl *>;

/// See `llvm::detail::indexed_accessor_range_base` for details.
static OwnerT offset_base(OwnerT object, ptrdiff_t index);
Expand Down
16 changes: 8 additions & 8 deletions mlir/include/mlir/IR/ValueRange.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const Value *, OpOperand *,
detail::OpResultImpl *, Value>,
Value, Value, Value> {
class ValueRange final
: public llvm::detail::indexed_accessor_range_base<
ValueRange,
PointerUnion<const Value *, OpOperand *, detail::OpResultImpl *>,
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<const Value *, OpOperand *, detail::OpResultImpl *, Value>;
PointerUnion<const Value *, OpOperand *, detail::OpResultImpl *>;

using RangeBaseT::RangeBaseT;

Expand All @@ -392,7 +392,7 @@ class ValueRange final : public llvm::detail::indexed_accessor_range_base<
std::is_constructible<ArrayRef<Value>, Arg>::value &&
!std::is_convertible<Arg, Value>::value>>
ValueRange(Arg &&arg) : ValueRange(ArrayRef<Value>(std::forward<Arg>(arg))) {}
ValueRange(Value value) : ValueRange(value, /*count=*/1) {}
ValueRange(const Value &value) : ValueRange(&value, /*count=*/1) {}
ValueRange(const std::initializer_list<Value> &values)
: ValueRange(ArrayRef<Value>(values)) {}
ValueRange(iterator_range<OperandRange::iterator> values)
Expand Down
13 changes: 0 additions & 13 deletions mlir/lib/IR/OperationSupport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value>(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<const Value *>(owner))
return {value + index};
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))
Expand All @@ -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<Value>(owner)) {
assert(index == 0 && "cannot offset into single-value 'ValueRange'");
return value;
}
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(owner))
return value[index];
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))
Expand Down
15 changes: 0 additions & 15 deletions mlir/lib/IR/TypeRange.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,23 +31,12 @@ TypeRange::TypeRange(ValueRange values) : TypeRange(OwnerT(), values.size()) {
this->base = result;
else if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(owner))
this->base = operand;
else if (auto value = llvm::dyn_cast_if_present<Value>(owner))
this->base = value.getType();
else
this->base = cast<const Value *>(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<Type>(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<const Value *>(object))
return {value + index};
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(object))
Expand All @@ -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<Type>(object)) {
assert(index == 0 && "cannot offset into single-value 'TypeRange'");
return type;
}
if (const auto *value = llvm::dyn_cast_if_present<const Value *>(object))
return (value + index)->getType();
if (auto *operand = llvm::dyn_cast_if_present<OpOperand *>(object))
Expand Down
17 changes: 0 additions & 17 deletions mlir/unittests/IR/OperationSupportTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 628976c

Please sign in to comment.