[PATCH] D145073: [InstCombine] fold signed absolute diff patterns

[Sanjay Patel via Phabricator via llvm-commits]

spatel planned changes to this revision.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:970
+  if (match(TVal, m_NSWSub(m_Specific(A), m_Specific(B))) &&
+      match(FVal, m_NSWSub(m_Specific(B), m_Specific(A))) &&
+      Pred == CmpInst::ICMP_SGT) {
----------------
RKSimon wrote:
> spatel wrote:
> > spatel wrote:
> > > goldstein.w.n wrote:
> > > > It also works for `nuw`: https://alive2.llvm.org/ce/z/YfWtvG (either op can be either `nsw`/`nuw`)
> > > Good point - the icmp+select filters out the poison either way. But if we match `nuw`, we can't propagate it:
> > > https://alive2.llvm.org/ce/z/2b5jiJ
> > > 
> > > So if the `sub` has >1 use, we'll need to clear the `nuw` on the existing `sub` or create a new instruction with `nsw` set. I don't think it's safe to alter the flags on the existing instruction if there's more than one use.
> > > 
> > > This might overlap with changes needed to support the "nabs" pattern that I mentioned in the patch description. Ok, if we make it a TODO for this patch? I'll add more tests either way.
> > "Alter" in that sentence should have been "set". We can always clear no-wrap flags safely, but we can't set them without some context.
> A TODO should be fine - thanks
On 2nd thought, this patch has a bug if the subtracts have both "nsw" and "nuw" set. So we need to do the more complicated logic in this step anyway. :)


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

https://reviews.llvm.org/D145073