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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added a comment.

In D59006#1438184 <https://reviews.llvm.org/D59006#1438184>, @lebedev.ri wrote:

> This LG, but i'm not sure i understand how this is related to D59066 <https://reviews.llvm.org/D59066>?
>  Here, we are clearly end up with no `select` in ASM.
>  But in D59066 <https://reviews.llvm.org/D59066> we expand to this pattern.
>  So there is something else that is able to do the transform that we do manually in D59066 <https://reviews.llvm.org/D59066>?
>  Should D59066 <https://reviews.llvm.org/D59066> be doing something else to simply trigger the existing transform?


Sorry - this sequence of patches got confusing. 
I started here just trying to improve x86 codegen, but then we thought other targets might benefit from something similar. 
So D59066 <https://reviews.llvm.org/D59066> was initially a superset of this change along with the select improvement. 
But then it became clear that the generic expansion is mostly as good as it could be - if you have a decent ISA, not SSE. :)
@nikic also added the select combine to make things generally better.
So, we removed the hacks for x86 from the other patch and hopefully made it clear in this patch that we are working around x86-specific potholes.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:23891
+  SDValue X = Op.getOperand(0);
+  SDValue Y = Op.getOperand(1);
   if (VT.getScalarType() == MVT::i1) {
----------------
nikic wrote:
> RKSimon wrote:
> > Move these down inside the "if (VT.is128BitVector())" loop ?
> Or also replace the usages in the i1 branch :)
Yes - that was the intent. I'll do that as a preliminary NFC.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:23912
+    if ((VT == MVT::v4i32 && !Subtarget.hasSSE41()) ||
+        (VT == MVT::v2i64 && !Subtarget.hasVLX())) {
+      SDLoc DL(Op);
----------------
nikic wrote:
> Instead of hardcoding specific types and subtargets, maybe check operation legality?
> 
> ```
> if (Op.getOpcode() == ISD::UADDSAT && !TLI.isOperationLegal(ISD::UMIN, VT)) {
>     // ...
> }
> if (Op.getOpcode() == ISD::USUBSAT && !TLI.isOperationLegal(ISD::UMAX, VT)) {
>     // ...
> }
> ```
Yes, that does look less fragile.


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

https://reviews.llvm.org/D59006