[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 ↗

(On Diff #268025)

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

1182 ↗

(On Diff #268025)

nit: to be one of

1183 ↗

(On Diff #268025)

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

1183 ↗

(On Diff #268025)

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

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());
+    llvm::erase_if(Vec, [](Attr *A) { return isa<T>(A); });
 
     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 arguments.  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 Second, typename... Rest, typename Y>
+LLVM_NODISCARD inline bool isa(const Y &Val) {
+  return isa<First>(Val) || isa<Second, 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))