[PATCH] D69245: [InstCombine] Canonicalize uadd.with.overflow to uadd.sat

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 24 12:00:41 PDT 2019


nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1744
+  if (!CondVal || !FalseVal)
+    return nullptr;
+
----------------
dmgreen wrote:
> nikic wrote:
> > dmgreen wrote:
> > > nikic wrote:
> > > > We might want to add an `m_ExtractValue<0>(Matcher)` matcher...
> > > Righteo.
> > > 
> > > I've only made it single index, as opposed to try and get fancy with some recursive templates. That is likely the most common case. I've also added a m_BinOpIntrinsic to more easily grab the BinaryOpIntrinsic.
> > I think it would make more sense to make this a matcher for WithOverflowInst intrinsics -- that's what's needed here, and matching both saturating and with.overflow in one matcher doesn't seem super useful in general.
> Oh yeah. That sounds like what I need. I could also add a generic "match one of these things" operator. Something like
> ```
> diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
> index 0cf8e62..649b047 100644
> --- a/llvm/include/llvm/IR/PatternMatch.h
> +++ b/llvm/include/llvm/IR/PatternMatch.h
> @@ -559,8 +559,9 @@ inline bind_ty<const Value> m_Value(const Value *&V) { return V; }
>  inline bind_ty<Instruction> m_Instruction(Instruction *&I) { return I; }
>  /// Match a binary operator, capturing it if we match.
>  inline bind_ty<BinaryOperator> m_BinOp(BinaryOperator *&I) { return I; }
> -/// Match a binary operator intrinsic, capturing it if we match.
> -inline bind_ty<BinaryOpIntrinsic> m_BinOpIntrinsic(BinaryOpIntrinsic *&I) { return I; }
> +/// Match a class of type T, capturing it if we match.
> +template <typename T>
> +inline bind_ty<T> m_Instruction(T *&I) { return I; }
>  
>  /// Match a ConstantInt, capturing the value if we match.
>  inline bind_ty<ConstantInt> m_ConstantInt(ConstantInt *&CI) { return CI; }
> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstC
> index f3b0ac5..46bfb81 100644
> --- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
> +++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
> @@ -1743,8 +1743,8 @@ foldOverflowingAddSubSelect(SelectInst &SI, InstCombiner::BuilderTy &Bui
>    Value *TrueVal = SI.getTrueValue();
>    Value *FalseVal = SI.getFalseValue();
>  
> -  BinaryOpIntrinsic *II;
> -  if (!match(CondVal, m_ExtractValue<1>(m_BinOpIntrinsic(II))) ||
> +  WithOverflowInst *II;
> +  if (!match(CondVal, m_ExtractValue<1>(m_Instruction(II))) ||
>        !match(FalseVal, m_ExtractValue<0>(m_Specific(II))))
>      return nullptr;
>  
> ```
> 
> What do you think? Is there a better name than m_Instruction? (It's better than my original m_OneOfTheseThings. I kind of feel like there should be some way of doing this already that I might be missing).
I like the idea. Overloading `m_Instruction()` for this purpose is probably not good though. Generally I'd prefer something that mentions the type on the matcher itself, i.e. `m_Instruction<WithOverflowInst>(II)`, though I guess that's more of usage style question, as it probably can't be enforced in the implementation.

Some ideas...

```
m_a<WithOverflowInst>(II)
m_isa<WithOverflowInst>(II)
m_InstanceOf<WithOverflowInst>(II)
m_OfType<WithOverflowInst>(II)
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69245/new/

https://reviews.llvm.org/D69245





More information about the llvm-commits mailing list