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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/include/llvm/IR/PatternMatch.h:561-563
+/// Match an instruction of type T, capturing it if we match.
+/// For example m_isa<WithOverflowInst>(Y)
+template <typename T> inline bind_ty<T> m_isa(T *&I) { return I; }
----------------
dmgreen wrote:
> spatel wrote:
> > dmgreen wrote:
> > > spatel wrote:
> > > > lebedev.ri wrote:
> > > > > dmgreen wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > nikic wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > I don't like this one.
> > > > > > > > > Can we change it to be a two-step process - just make `m_isa<>()` a predicate,
> > > > > > > > > if `isa<>()` matches, then it should apply inner matcher, if any.
> > > > > > > > > And said inner matcher can be `m_Value()`.
> > > > > > > > I'm not sure I understand how that would be used. If you use something like `m_isa<WithOverflowInst>(m_Value(V))`, wouldn't `V` have to be `Value *` rather than `WithOverflowInst *`? That seems less useful to me. It's also not clear what things apart from `m_Value()` you would use as the inner matcher.
> > > > > > > Good point. I'm mainly unhappy about the name.
> > > > > > > `m_isa<>()` really looks similar to normal `isa<>()`, from looking at it i would not
> > > > > > > have guessed it takes reference to the pointer of that type and binds to it if matched.
> > > > > > Right. I did think of that but is seemed OK as it was in the middle of a match statement.
> > > > > > 
> > > > > > Got a better suggestion, or a preference out of these?
> > > > > > ```
> > > > > > m_a<WithOverflowInst>(II)
> > > > > > m_InstanceOf<WithOverflowInst>(II)
> > > > > > m_OfType<WithOverflowInst>(II)
> > > > > > m_OneOfTheseThings<WithOverflowInst>(II)
> > > > > > ```
> > > > > To be honest, none of those tell me that they capture..
> > > > > Would `m_CaptureIf_isa<>()` be too ugly?
> > > > > @RKSimon @spatel thoughts?
> > > > Catching up after travel...I might have missed something in the earlier inline comments, but we just need a WithOverflowInst sibling to:
> > > >   inline bind_ty<BinaryOperator> m_BinOp(BinaryOperator *&I) { return I; } 
> > > > ...for this patch, right? 
> > > > 
> > > > It's not clear to me that a more templated generic binder is needed. Either way, it would be good to split the matcher changes from the instcombine changes, so there's less risk of losing improvements if some part of this needs to be reverted.
> > > Do you mean like this version: https://reviews.llvm.org/D69245?id=226110 :)
> > > 
> > > We can go back to that. I quite like the ability to capture a thing of type T, but you are correct that it is simpler to just add the m_WithOverflowInst case.
> > Yes, that's the simpler solution since we don't have a clear winner for the more generic matcher.
> > I'd still split this when committing (adding a couple of dedicated tests to unittests/IR/PatternMatch.cpp is even better), but either way, LGTM.
> Part1: https://reviews.llvm.org/rG6cfbefbc4a7e. Let me know if that test looks dodgy.
Thanks! Unit tests LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69245