[LLVM] Change isa<> to a variadic function template

[jurahul]

Summary

Change isa<> to a variadic function template, so that it can be used to test against one of multiple types as follows:

isa<Type0, Type1, Type2>(Val)

[rriddle]

Thanks, added a couple of comments.

mlir/include/mlir/IR/OpDefinition.h

1151

Can you add this to llvm/Support/Casting.h? This is something I've been meaning to do for a while.

1155

nit: to be one of

1156

Can you just use a fold expression instead?

I would expect one of the following to work:

diag << llvm::makeArrayRef({ParentOpTypes::getOperationName()...});

llvm::interleaveComma(llvm::makeArrayRef({ParentOpTypes::getOperationName()...}), diag);

[mehdi_amini]

mlir/include/mlir/IR/OpDefinition.h

1156

Wow! I thought we needed C++17 for such folding...

[jurahul]

Address River's comments

[jurahul]

Attempt to fix SCEV build failure on Windows.

[jurahul]

Another attempt to address Windows build failures.

[jurahul]

Another fix for Windows build (really need to setup a Windows build :( )

[jurahul]

@rriddle and others, I have extended the isa<> function to be a variadic template, but that is causing a ripple effect of several places breaking where isa<> is used as a predicate and instantiate using something like:

if (SCEVExprContains(T, isa<SCEVUnknown, const SCEV *>))

with the new isa<> definition, such code does not work as is and needs to updated. I am working on fixing all references, but I am wondering if this will cause churn in out-of-tree code that uses LLVM as well. I cannot call the new functions is_one_of as there already exists a struct of this type with a different meaning. Another option is to name it isa_one_of but that does not read well I think. Do you agree with extending isa<> as in this change? If so, I can continue patching the remaining places (once I setup a windows build).

[jurahul]

Fix Clang build failure

[jurahul]

Fix Clang build failure

[jurahul]

fix MLIR test failure due to change in error message. Reinstate the old message for a single parent case

[jurahul]

This is ready for review. The 2 Clang hip related failure are not caused by this change, and the offending change has been reverted now (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20200601/323475.html)

[jurahul]

Remove {} in ScalarEvolution.cpp to match existing style

[jpienaar]

llvm/include/llvm/Support/Casting.h

147

I'm worried that folks may not notice this change based on the change description (e.g., the change sounds MLIR specific), could you make this a separate change? (just focused on isa changes)

[jurahul]

llvm/include/llvm/Support/Casting.h

147

I did think of doing that. I agree that we should split the isa<> into its own change.

[jurahul]

Split isa<> changes into its own change

[rriddle]

I have run into the desire to have this on many occasions, so LGTM for me.

clang/include/clang/AST/DeclBase.h

521–523

nit: We could also update this to use llvm::erase_if, but fine to leave as is.

llvm/include/llvm/Support/Casting.h

136

nit: arguments

147

nit: I would remove the enable_if and just add an additional type to the template.

template<typename First, typename Second, ...>
... isa(const Y &val) {
  return isa<First>(Val) || isa<Second, Rest...>(Val);
}

[jurahul]

Address review comments

[mehdi_amini]

Do you need someone to land this for you?

[jurahul]

Yes, I'd appreciate if someone can land this change.

clang/include/clang/AST/DeclBase.h

attr_iterator attr_end() const {
     return hasAttrs() ? getAttrs().end() : nullptr;
   }
 
   template <typename T>
   void dropAttr() {
     if (!HasAttrs) return;
 
     AttrVec &Vec = getAttrs();
-    Vec.erase(std::remove_if(Vec.begin(), Vec.end(), isa<T, Attr*>), Vec.end());
+    Vec.erase(std::remove_if(Vec.begin(), Vec.end(),
+                             [](Attr *A) { return isa<T>(A); }),
+              Vec.end());
 
     if (Vec.empty())
       HasAttrs = false;
   }
 
   template <typename T>
   llvm::iterator_range<specific_attr_iterator<T>> specific_attrs() const {
     return llvm::make_range(specific_attr_begin<T>(), specific_attr_end<T>());

llvm/include/llvm/Support/Casting.h

 template<typename To, typename FromTy>
 struct isa_impl_wrap<To, FromTy, FromTy> {
   // When From == SimpleType, we are as simple as we are going to get.
   static bool doit(const FromTy &Val) {
     return isa_impl_cl<To,FromTy>::doit(Val);
   }
 };
 
-// isa<X> - Return true if the parameter to the template is an instance of the
-// template type argument.  Used like this:
+// isa<X> - Return true if the parameter to the template is an instance of one
+// of the template type argument.  Used like this:
 //
 //  if (isa<Type>(myVal)) { ... }
+//  if (isa<Type0, Type1, Type2>(myVal)) { ... }
 //
 template <class X, class Y> LLVM_NODISCARD inline bool isa(const Y &Val) {
   return isa_impl_wrap<X, const Y,
                        typename simplify_type<const Y>::SimpleType>::doit(Val);
 }
 
+template <typename First, typename... Rest, typename Y>
+LLVM_NODISCARD inline typename std::enable_if<sizeof...(Rest) != 0, bool>::type
+isa(const Y &Val) {
+  return isa<First>(Val) || isa<Rest...>(Val);
+}
+
 // isa_and_nonnull<X> - Functionally identical to isa, except that a null value
 // is accepted.
 //
-template <class X, class Y>
+template <typename... X, class Y>
 LLVM_NODISCARD inline bool isa_and_nonnull(const Y &Val) {
   if (!Val)
     return false;
-  return isa<X>(Val);
+  return isa<X...>(Val);
 }
 
 //===----------------------------------------------------------------------===//
 //                          cast<x> Support Templates
 //===----------------------------------------------------------------------===//
 
 template<class To, class From> struct cast_retty;
 

llvm/lib/Analysis/ScalarEvolution.cpp

bool ScalarEvolution::checkValidity(const SCEV *S) const {
   return !ContainsNulls;
 }
 
 bool ScalarEvolution::containsAddRecurrence(const SCEV *S) {
   HasRecMapType::iterator I = HasRecMap.find(S);
   if (I != HasRecMap.end())
     return I->second;
 
-  bool FoundAddRec = SCEVExprContains(S, isa<SCEVAddRecExpr, const SCEV *>);
+  bool FoundAddRec =
+      SCEVExprContains(S, [](const SCEV *S) { return isa<SCEVAddRecExpr>(S); });
   HasRecMap.insert({S, FoundAddRec});
   return FoundAddRec;
 }
 
 /// Try to split a SCEVAddExpr into a pair of {SCEV, ConstantInt}.
 /// If \p S is a SCEVAddExpr and is composed of a sub SCEV S' and an
 /// offset I, then return {S', I}, else return {\p S, nullptr}.
 static std::pair<const SCEV *, ConstantInt *> splitAddExpr(const SCEV *S) {
static bool findArrayDimensionsRec(ScalarEvolution &SE,
 
   Sizes.push_back(Step);
   return true;
 }
 
 // Returns true when one of the SCEVs of Terms contains a SCEVUnknown parameter.
 static inline bool containsParameters(SmallVectorImpl<const SCEV *> &Terms) {
   for (const SCEV *T : Terms)
-    if (SCEVExprContains(T, isa<SCEVUnknown, const SCEV *>))
+    if (SCEVExprContains(T, [](const SCEV *S) { return isa<SCEVUnknown>(S); }))
       return true;
+
   return false;
 }
 
 // Return the number of product terms in S.
 static inline int numberOfTerms(const SCEV *S) {
   if (const SCEVMulExpr *Expr = dyn_cast<SCEVMulExpr>(S))
     return Expr->getNumOperands();
   return 1;

llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp

bool runOnFunction(Function &F) override {
     return Changed;
   }
 
   bool runOnUse(Use &U, const PHINodeSet &PromotablePHINodes,
                        B2IMap &BoolToIntMap) {
     auto Defs = findAllDefs(U);
 
     // If the values are all Constants or Arguments, don't bother
-    if (llvm::none_of(Defs, isa<Instruction, Value *>))
+    if (llvm::none_of(Defs, [](Value *V) { return isa<Instruction>(V); }))
       return false;
 
     // Presently, we only know how to handle PHINode, Constant, Arguments and
     // CallInst. Potentially, bitwise operations (AND, OR, XOR, NOT) and sign
     // extension could also be handled in the future.
     for (Value *V : Defs)
       if (!isa<PHINode>(V) && !isa<Constant>(V) &&
           !isa<Argument>(V) && !isa<CallInst>(V))

mlir/include/mlir/IR/OpBase.td

 // Op's regions have a single block with the specified terminator.
 class SingleBlockImplicitTerminator<string op>
     : ParamNativeOpTrait<"SingleBlockImplicitTerminator", op>;
 
 // Op's parent operation is the provided one.
 class HasParent<string op>
     : ParamNativeOpTrait<"HasParent", op>;
 
+class ParentOneOf<list<string> ops>
+    : ParamNativeOpTrait<"HasParent", StrJoin<ops>.result>;
+
 // Op result type is derived from the first attribute. If the attribute is an
 // subclass of `TypeAttrBase`, its value is used, otherwise, the type of the
 // attribute content is used.
 def FirstAttrDerivedResultType :
   GenInternalOpTrait<"FirstAttrDerivedResultType">;
 
 // TODO(antiagainst): Turn the following into normal traits and generate
 // verification for them.

mlir/include/mlir/IR/OpDefinition.h

public:
     Block *getBody(unsigned idx = 0) {
       Region &region = this->getOperation()->getRegion(idx);
       assert(!region.empty() && "unexpected empty region");
       return &region.front();
     }
   };
 };
 
-/// This class provides a verifier for ops that are expecting a specific parent.
-template <typename ParentOpType> struct HasParent {
+/// This class provides a verifier for ops that are expecting their parent
+/// to be one of the given parent ops
+template <typename... ParentOpTypes>
+struct HasParent {
   template <typename ConcreteType>
   class Impl : public TraitBase<ConcreteType, Impl> {
   public:
     static LogicalResult verifyTrait(Operation *op) {
-      if (isa<ParentOpType>(op->getParentOp()))
+      if (llvm::isa<ParentOpTypes...>(op->getParentOp()))
         return success();
-      return op->emitOpError() << "expects parent op '"
-                               << ParentOpType::getOperationName() << "'";
+
+      InFlightDiagnostic diag = op->emitOpError();
+      diag << "expects parent op "
+           << (sizeof...(ParentOpTypes) != 1 ? "to be one of '" : "'");
+      llvm::interleaveComma(
+          llvm::makeArrayRef({ParentOpTypes::getOperationName()...}), diag);
+      diag << "'";
+      return diag;
     }
   };
 };
 
 /// A trait for operations that have an attribute specifying operand segments.
 ///
 /// Certain operations can have multiple variadic operands and their size
 /// relationship is not always known statically. For such cases, we need

mlir/test/IR/traits.mlir

func @failedHasParent_wrong_parent() {
   "some.op"() ({
    // expected-error@+1 {{'test.child' op expects parent op 'test.parent'}}
     "test.child"() : () -> ()
   }) : () -> ()
 }
 
 // -----
 
+// CHECK: succeededParentOneOf
+func @succeededParentOneOf() {
+  "test.parent"() ({
+    "test.child_with_parent_one_of"() : () -> ()
+    "test.finish"() : () -> ()
+   }) : () -> ()
+  return
+}
+
+// -----
+
+// CHECK: succeededParent1OneOf
+func @succeededParent1OneOf() {
+  "test.parent1"() ({
+    "test.child_with_parent_one_of"() : () -> ()
+    "test.finish"() : () -> ()
+   }) : () -> ()
+  return
+}
+
+// -----
+
+func @failedParentOneOf_wrong_parent1() {
+  "some.otherop"() ({
+    // expected-error@+1 {{'test.child_with_parent_one_of' op expects parent op to be one of 'test.parent, test.parent1'}}
+    "test.child_with_parent_one_of"() : () -> ()
+    "test.finish"() : () -> ()
+   }) : () -> ()
+}
+
+
+// -----
+
 func @failedSingleBlockImplicitTerminator_empty_block() {
    // expected-error@+1 {{'test.SingleBlockImplicitTerminator' op expects a non-empty block}}
   "test.SingleBlockImplicitTerminator"() ({
   ^entry:
   }) : () -> ()
 }
 
 // -----

mlir/test/lib/Dialect/Test/TestOps.td

def IfFirstOperandIsNoneThenSoIsSecond :
   let arguments = (ins AnyType:$x, AnyType:$y);
 }
 
 def BroadcastableOp : TEST_Op<"broadcastable", [ResultsBroadcastableShape]> {
   let arguments = (ins Variadic<AnyTensor>);
   let results = (outs AnyTensor);
 }
 
-// There the "HasParent" trait.
-def ParentOp : TEST_Op<"parent">;
+// HasParent trait
+def ParentOp : TEST_Op<"parent"> {
+    let regions = (region AnyRegion);
+}
 def ChildOp : TEST_Op<"child", [HasParent<"ParentOp">]>;
 
+// ParentOneOf trait
+def ParentOp1 : TEST_Op<"parent1"> {
+  let regions = (region AnyRegion);
+}
+def ChildWithParentOneOf : TEST_Op<"child_with_parent_one_of",
+                                [ParentOneOf<["ParentOp", "ParentOp1"]>]>;
 
 def TerminatorOp : TEST_Op<"finish", [Terminator]>;
 def SingleBlockImplicitTerminatorOp : TEST_Op<"SingleBlockImplicitTerminator",
     [SingleBlockImplicitTerminator<"TerminatorOp">]> {
   let regions = (region SizedRegion<1>:$region);
 }
 
 def I32ElementsAttrOp : TEST_Op<"i32ElementsAttr"> {