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

[Dave Green via Phabricator via llvm-commits]

dmgreen marked an inline comment as done.
dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1744
+  if (!CondVal || !FalseVal)
+    return nullptr;
+
----------------
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).


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

https://reviews.llvm.org/D69245