[PATCH] D59006: [x86] improve the default expansion of uaddsat/usubsat

[Sanjay Patel via Phabricator via llvm-commits]

spatel planned changes to this revision.
spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:36147
+    return DAG.getSelect(DL, VT, Cmp, DAG.getAllOnesConstant(DL, VT), Add);
+  }
+
----------------
RKSimon wrote:
> This only differs from the default expansion by the optimal CondCode to use in the select - ideally we'd have a way for TLI to indicate 'preferred' comparison codes - x86/sse is probably not alone in having limited comparisons (SGT + EQ) and the others  having to be custom handled.
Yes, if we can thread the predicate needle, we can get the optimal x86 code by changing the generic expansion, rather than adding x86-specific combines.

Looking at this a bit closer: the key to making this generically better is realizing that this select shouldn't be a select if we have a vector 0/-1 mask created by the compare. In that case, we should only have a bitwise logic op (and/or), never a pblendv or pandn.

Unfortunately, it seems we're missing some generic and/or x86-specific min/max transforms to back that up, so I need to chase those down.

We may also be suffering from the fact that D58974 is not a generic combine. Let me know if I should deal with that one.

@nikic - I just want to confirm: are the addsat/subsat tests in trunk only for x86 right now? If so, I want to duplicate them for AArch64 at least, so we know that we're making things better for more than just x86. 


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

https://reviews.llvm.org/D59006