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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:36147
+    return DAG.getSelect(DL, VT, Cmp, DAG.getAllOnesConstant(DL, VT), Add);
+  }
+
----------------
spatel wrote:
> 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. 
> 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.

Yes, right now we're testing codegen only for X86. I was planning to look into better AArch64 codegen soon. It's probably not a good target to test generic expansions, because iirc it has instructions covering the full set of legal vector types.


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

https://reviews.llvm.org/D59006