This is an archive of the discontinued LLVM Phabricator instance.

Differential D125543

[mlir] support isa/cast/dyn_cast<Operation *>(operation) again
ClosedPublic

Authored by ftynse on May 13 2022, 6:11 AM.

Details

Reviewers
rriddle
bzcheeseman
Commits
rG1075c8ca49b4: [mlir] support isa/cast/dyn_cast<Operation *>(operation) again
Summary

The support for this has been added by 946311b8938114a37db5c9d42fb9f5a1481ccae1
but then ignored by bc22b5c9a2f729460ffdf7627b3534a8d9f3f767.

This enables one to write generic code that can be instantiated for both
specific operation classes and the common base class without
specialization. Examples include functions that take/return ops, such
as:

mlir
template <typename FnTy>
void applyIf(FnTy &&lambda, ...) {
  for (Operation *op : ...) {
    auto specific = dyn_cast<function_traits<FnTy>::template arg_t<0>>(op);
    if (specific)
      lambda(specific);
  }
}

that would otherwise need to rely on template specialization to support
lambdas that take specific operations and those that take Operation *.

Diff Detail

Event Timeline

ftynse created this revision.May 13 2022, 6:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 6:11 AM
Herald added subscribers: sdasgup3, wenzhicui, wrengr and 19 others. · View Herald Transcript
ftynse requested review of this revision.May 13 2022, 6:11 AM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald TranscriptMay 13 2022, 6:11 AM
Harbormaster completed remote builds in B164298: Diff 429219.May 13 2022, 6:42 AM
rriddle added a reviewer: bzcheeseman.May 13 2022, 10:10 AM

@bzcheeseman Can you advise if this is the easiest way to do a "self" cast?

bzcheeseman added a comment.EditedMay 13 2022, 10:13 AM

Actually it'd probably be easier to just add an if (std::is_same<T, ::mlir::Operation *>::value) return true; condition to isPossible in the non-const casts, everything else should "just work".

I thought I had added this, I guess not! Thanks for adding it back :)

bzcheeseman added a comment.May 13 2022, 3:27 PM

Actually it'll probably be better to have a CastInfo specialization for all self-casts. I'll put up a patch hopefully shortly.

bzcheeseman mentioned this in D125590: [LLVM][Casting.h] Add trivial self-cast.May 13 2022, 3:50 PM
bzcheeseman added a comment.May 13 2022, 3:52 PM

@ftynse PTAL at https://reviews.llvm.org/D125590 - hopefully that will just solve the problem.

bzcheeseman mentioned this in rG0809f63826d3: [LLVM][Casting.h] Add trivial self-cast.May 15 2022, 10:22 PM
bzcheeseman added a comment.May 16 2022, 11:01 AM

@ftynse https://reviews.llvm.org/D125590 should fix this - can you please confirm and let me know if not?

dcaballe added a comment.May 16 2022, 5:24 PM

I can confirm that D125590 is not fixing the problem. D123901 broke our mlir-based project (IREE) due to an Operation * self-cast. Only with the proposed changes in this diff things get back to work.

More info about the error:

.../iree/third_party/llvm-project/mlir/include/mlir/IR/Operation.h:815:58: error: type 'mlir::Operation *' cannot be used prior to '::' because it has no members
  static bool isPossible(::mlir::Operation *op) { return T::classof(op); }
                                                         ^
.../iree/third_party/llvm-project/llvm/include/llvm/Support/Casting.h:319:19: note: in instantiation of member function 'llvm::CastInfo<mlir::Operation *, mlir::Operation *>::isPossible' requested here
    if (!Derived::isPossible(f))
                  ^
.../iree/third_party/llvm-project/llvm/include/llvm/Support/Casting.h:611:32: note: in instantiation of member function 'llvm::DefaultDoCastIfPossible<mlir::Operation *, mlir::Operation *, llvm::CastInfo<mlir::Operation *, mlir::Operatio
n *>>::doCastIfPossible' requested here
  return CastInfo<To, From *>::doCastIfPossible(Val);
                               ^
.../iree/llvm-external-projects/iree-dialects/include/iree-dialects/Dialect/LinalgTransform/TransformOpTraits.h:98:24: note: in instantiation of function template specialization 'mlir::transform::detail::applyTransformToEach<(lambda at /
.../iree/llvm-external-projects/iree-dialects/include/iree-dialects/Dialect/LinalgTransform/TransformOpTraits.h:99:31)>' requested here
    if (failed(detail::applyTransformToEach(
                       ^
.../iree/build/release/third_party/llvm-project/llvm/tools/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h.inc:55:56: note: in instantiation of member function 'mlir::transform::TargetableSingleOperandOpTrait<transform_ext::
GetParentLoopOp>::apply' requested here
  return (llvm::cast<ConcreteOp>(tablegen_opaque_val)).apply(transformResults, state);
                                                       ^
.../iree/build/release/third_party/llvm-project/llvm/tools/mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h.inc:21:23: note: in instantiation of member function 'mlir::transform::detail::TransformOpInterfaceInterfaceTraits::M
odel<transform_ext::GetParentLoopOp>::apply' requested here
    Model() : Concept{apply} {}
                      ^

Hopefully, we can land this soon.

rriddle accepted this revision.May 16 2022, 6:22 PM

I'm fine with landing this to unbreak things, but we really shouldn't need to define this IMO (the infra should automatically handle it).

This revision is now accepted and ready to land.May 16 2022, 6:22 PM
bzcheeseman added a comment.May 16 2022, 8:51 PM
In D125543#3517778, @rriddle wrote:

I'm fine with landing this to unbreak things, but we really shouldn't need to define this IMO (the infra should automatically handle it).

Agreed. Can we get some way to repro this so we can do it in the infra?

ftynse added a comment.May 17 2022, 1:47 AM

The minimal example is

#include "mlir/IR/Operation.h"

using namespace mlir;

Operation *dynCastOperation(Operation *op) {
  return dyn_cast<Operation *>(op);
}

which fails to compile at llvmorg-15-init-10618-g8d6e2c3e3db1 with

~/llvm-project/mlir/include/mlir/IR/Operation.h:815:58: error: type 'mlir::Operation *' cannot be used prior to '::' because it has no members
  static bool isPossible(::mlir::Operation *op) { return T::classof(op); }
                                                         ^
~/llvm-project/llvm/include/llvm/Support/Casting.h:319:19: note: in instantiation of member function 'llvm::CastInfo<mlir::Operation *, mlir::Operation *, void>::isPossible' requested here
    if (!Derived::isPossible(f))
                  ^
~/llvm-project/llvm/include/llvm/Support/Casting.h:611:32: note: in instantiation of member function 'llvm::DefaultDoCastIfPossible<mlir::Operation *, mlir::Operation *, llvm::CastInfo<mlir::Operation *, mlir::Operation *, void> >::doCastIfPossible' requested here
  return CastInfo<To, From *>::doCastIfPossible(Val);
ftynse added a comment.May 17 2022, 1:58 AM

This will not happen for a random class that does not have CastInfo specialized though. For Operation *, the compiler picks the CastInfo<T, ::mlir::Operation *> specialization over the more generic CastInfo<T, T>, I don't think there is an easy way around this.

ftynse added a comment.May 17 2022, 2:02 AM
In D125543#3512054, @bzcheeseman wrote:

Actually it'd probably be easier to just add an if (std::is_same<T, ::mlir::Operation *>::value) return true; condition to isPossible in the non-const casts, everything else should "just work".

This will not work. The instantiated code will still contain the T::classof that will be instantiated as the offending Operation *::classof.

Closed by commit rG1075c8ca49b4: [mlir] support isa/cast/dyn_cast<Operation *>(operation) again (authored by ftynse). · Explain WhyMay 17 2022, 2:15 AM
This revision was automatically updated to reflect the committed changes.
ftynse added a commit: rG1075c8ca49b4: [mlir] support isa/cast/dyn_cast<Operation *>(operation) again.
bzcheeseman added a comment.May 17 2022, 7:03 AM
In D125543#3518277, @ftynse wrote:

This will not happen for a random class that does not have CastInfo specialized though. For Operation *, the compiler picks the CastInfo<T, ::mlir::Operation *> specialization over the more generic CastInfo<T, T>, I don't think there is an easy way around this.

Interesting! Thanks for clarifying (and for the repro case).

Revision Contents

PathSize
mlir/
include/
mlir/
IR/
18 lines

Diff 429971

mlir/include/mlir/IR/Operation.h

Show First 20 Lines • Show All 828 Lines • ▼ Show 20 Linesstruct CastInfo<T, ::mlir::Operation>
// ConstStrippingCast. // ConstStrippingCast.
static bool isPossible(::mlir::Operation &val) { return T::classof(&val); } static bool isPossible(::mlir::Operation &val) { return T::classof(&val); }
static T doCast(::mlir::Operation &val) { return T(&val); } static T doCast(::mlir::Operation &val) { return T(&val); }
}; };
template <typename T> template <typename T>
struct CastInfo<T, const ::mlir::Operation> struct CastInfo<T, const ::mlir::Operation>
: public ConstStrippingForwardingCast<T, const ::mlir::Operation, : public ConstStrippingForwardingCast<T, const ::mlir::Operation,
CastInfo<T, ::mlir::Operation>> {}; CastInfo<T, ::mlir::Operation>> {};
/// Cast (const) Operation * to itself. This is helpful to avoid SFINAE in
/// templated implementations that should work on both base and derived
/// operation types.
template <>
struct CastInfo<::mlir::Operation *, ::mlir::Operation *>
: public NullableValueCastFailed<::mlir::Operation *>,
public DefaultDoCastIfPossible<
::mlir::Operation *, ::mlir::Operation *,
CastInfo<::mlir::Operation *, ::mlir::Operation *>> {
static bool isPossible(::mlir::Operation *op) { return true; }
static ::mlir::Operation *doCast(::mlir::Operation *op) { return op; }
};
template <>
struct CastInfo<const ::mlir::Operation *, const ::mlir::Operation *>
: public ConstStrippingForwardingCast<
const ::mlir::Operation *, const ::mlir::Operation *,
CastInfo<::mlir::Operation *, ::mlir::Operation *>> {};
} // namespace llvm } // namespace llvm
#endif // MLIR_IR_OPERATION_H #endif // MLIR_IR_OPERATION_H