[llvm] r223785 - Add more pattern matchers for compares, instructions, and BinaryOperators. NFC.

[Chandler Carruth]

On Tue, Dec 9, 2014 at 5:36 PM, Juergen Ributzka <juergen at apple.com> wrote:

> Author: ributzka
> Date: Tue Dec  9 10:36:10 2014
> New Revision: 223785
>
> URL: http://llvm.org/viewvc/llvm-project?rev=223785&view=rev
> Log:
> Add more pattern matchers for compares, instructions, and BinaryOperators.
> NFC.
>
> Add a few more matchers to make the code in the next commit more compact.
>
> Modified:
>     llvm/trunk/include/llvm/IR/PatternMatch.h
>
> Modified: llvm/trunk/include/llvm/IR/PatternMatch.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/PatternMatch.h?rev=223785&r1=223784&r2=223785&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/PatternMatch.h (original)
> +++ llvm/trunk/include/llvm/IR/PatternMatch.h Tue Dec  9 10:36:10 2014
> @@ -68,6 +68,18 @@ struct class_match {
>
>  /// m_Value() - Match an arbitrary value and ignore it.
>  inline class_match<Value> m_Value() { return class_match<Value>(); }
>

Please keep a blank line between top-level constructs, especially those
preceded by comments, especially doxygen comments. It makes reading *much*
easier.


> +/// m_Instruction() - Match an arbitrary instruction and ignore it.
>

Please don't repeat function names in doxygen comments. I know lots of code
does this, but new code shouldn't.

I'm also surprised at the instruction matcher. Why would Value not work?
Almost all of pattern match works very hard to match generically across
both instructions and constants, and it seems strange to start diverging
now.


> +inline class_match<Instruction> m_Instruction() {
> +  return class_match<Instruction>();
> +}
> +/// m_BinOp() - Match an arbitrary binary operation and ignore it.
> +inline class_match<BinaryOperator> m_BinOp() {
> +  return class_match<BinaryOperator>();
> +}
> +/// m_Cmp() - Matches any compare instruction and ignore it.
> +inline class_match<CmpInst> m_Cmp() {
> +  return class_match<CmpInst>();
> +}
>

I see you're just replicating other matchers. I think it makes a lot more
sense to organize them together. For example, having this here, and then a
capturing matcher below, and then a *non* capturing matcher that works
totally differently from this still further down doesn't really make sense
to me.

What this really indicates is a fundamental flaw in the matcher interface:
it is not well suited to matching instructions when you need to capture
them while matching. I tried to use it to do that just the other day as
well, and wrote code to extend it in similar ways, but the code piles up
and doesn't really make sense. And it doesn't make the call sites that much
better either.

With the current design of the pattern match library, I don't think it
makes sense to do non-leaf-matching patterns with it. To do those, you
really want to decouple the pattern structure from the binding action (much
as we did with the ASTMatchers in Clang) but that's a totally new API.


>  /// m_ConstantInt() - Match an arbitrary ConstantInt and ignore it.
>  inline class_match<ConstantInt> m_ConstantInt() {
>    return class_match<ConstantInt>();
> @@ -299,6 +311,12 @@ struct bind_ty {
>  /// m_Value - Match a value, capturing it if we match.
>  inline bind_ty<Value> m_Value(Value *&V) { return V; }
>
> +/// m_Instruction - Match a instruction, capturing it if we match.
> +inline bind_ty<Instruction> m_Instruction(Instruction *&I) { return I; }
> +
> +/// m_BinOp - Match a instruction, capturing it if we match.
> +inline bind_ty<BinaryOperator> m_BinOp(BinaryOperator *&I) { return I; }
> +
>  /// m_ConstantInt - Match a ConstantInt, capturing the value if we match.
>  inline bind_ty<ConstantInt> m_ConstantInt(ConstantInt *&CI) { return CI; }
>
> @@ -390,6 +408,30 @@ inline specific_intval m_SpecificInt(uin
>  inline bind_const_intval_ty m_ConstantInt(uint64_t &V) { return V; }
>
>
>  //===----------------------------------------------------------------------===//
> +// Matcher for any binary operator.
> +//
> +template<typename LHS_t, typename RHS_t>
> +struct AnyBinaryOp_match {
> +  LHS_t L;
> +  RHS_t R;
> +
> +  AnyBinaryOp_match(const LHS_t &LHS, const RHS_t &RHS) : L(LHS), R(RHS)
> {}
> +
> +  template<typename OpTy>
> +  bool match(OpTy *V) {
> +    if (auto *I = dyn_cast<BinaryOperator>(V))
> +      return L.match(I->getOperand(0)) && R.match(I->getOperand(1));
> +    return false;
> +  }
> +};
>

Having completely separate code for matching any binary operators and
specific binary operators really doesn't make sense to me. Why not combine
them?


> +
> +template<typename LHS, typename RHS>
> +inline AnyBinaryOp_match<LHS, RHS>
> +m_BinOp(const LHS &L, const RHS &R) {
> +  return AnyBinaryOp_match<LHS, RHS>(L, R);
> +}
> +
>
> +//===----------------------------------------------------------------------===//
>  // Matchers for specific binary operators.
>  //
>
> @@ -701,17 +743,21 @@ struct CmpClass_match {
>  };
>
>  template<typename LHS, typename RHS>
> +inline CmpClass_match<LHS, RHS, CmpInst, CmpInst::Predicate>
> +m_Cmp(CmpInst::Predicate &Pred, const LHS &L, const RHS &R) {
> +  return CmpClass_match<LHS, RHS, CmpInst, CmpInst::Predicate>(Pred, L,
> R);
> +}
> +
> +template<typename LHS, typename RHS>
>  inline CmpClass_match<LHS, RHS, ICmpInst, ICmpInst::Predicate>
>  m_ICmp(ICmpInst::Predicate &Pred, const LHS &L, const RHS &R) {
> -  return CmpClass_match<LHS, RHS,
> -                        ICmpInst, ICmpInst::Predicate>(Pred, L, R);
> +  return CmpClass_match<LHS, RHS, ICmpInst, ICmpInst::Predicate>(Pred, L,
> R);
>  }
>
>  template<typename LHS, typename RHS>
>  inline CmpClass_match<LHS, RHS, FCmpInst, FCmpInst::Predicate>
>  m_FCmp(FCmpInst::Predicate &Pred, const LHS &L, const RHS &R) {
> -  return CmpClass_match<LHS, RHS,
> -                        FCmpInst, FCmpInst::Predicate>(Pred, L, R);
> +  return CmpClass_match<LHS, RHS, FCmpInst, FCmpInst::Predicate>(Pred, L,
> R);
>  }
>
>
>  //===----------------------------------------------------------------------===//
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141209/fc71a778/attachment.html>