[PATCH] D69514: [InstCombine] Expand usub_sat patterns to handle constants

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:722
   // afterwards.
-  if (!TrueVal->hasOneUse())
+  if (!TrueVal->hasOneUse() || !ICI->hasOneUse())
     return nullptr;
----------------
dmgreen wrote:
> lebedev.ri wrote:
> > Err, this isn't quite right. I should have been more specific.
> > 
> > We produce a single instruction `@llvm.usub.sat.`,
> > so there shouldn't be a one-use check in general.
> > 
> > But we also may need to negate the result.
> > So *if* we need to negate the result,
> > *then* we need ensure that a *single* instruction
> > goes away - **either** `icmp` **or** `sub`.
> > 
> OK. I see. I was thinking you were referring to assembly instructions being longer if the icmp had multiple uses.
> 
> There may be some benefit to making sure the output is actually smaller.
> 
> I've updated the code. Let me know if this was or wasn't what you were thinking.
Yep, this seems what i meant.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:707
 
   // Account for swapped form of subtraction: ((a > b) ? b - a : 0).
+  // Checking for both a-b and a+(-b) as a constant.
----------------
I'm having doubt about the comment.
If `b < a`, then `b - a` will wrap, and we'll return wrapped result.
And if `b >= a`, then `b - a` will **not** wrap, but we will saturate to `0`.
What am i missing?


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

https://reviews.llvm.org/D69514