[PATCH] D56636: [CodeGen][X86] Expand vector USUBSAT to UMAX+SUB

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 4 inline comments as done.
nikic added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:334
   case ISD::ADD:
   case ISD::SUB:
   case ISD::MUL:
----------------
RKSimon wrote:
> Add ISD::USUBSAT et al here as well?
It's already present a bit lower (L413).


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:5317
+    // usub.sat(a, b) -> umax(a, b) - b
+    if (Opcode == ISD::USUBSAT && isOperationLegal(ISD::UMAX, VT)) {
+      SDValue Max = DAG.getNode(ISD::UMAX, dl, VT, LHS, RHS);
----------------
RKSimon wrote:
> isOperationLegalOrCustom?
Extra diff allowing custom: https://gist.github.com/nikic/4c46634cec8f319e687c6b5cb0496648

This is presumably better than scalarizing, but I was wondering if a more explicit expansion would work better?

As another variant, this is what happens if I just fall through to the USUBO+SELECT expansion: https://gist.github.com/nikic/d989121f7f9898437a1255548c148904


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:1224
+    if (!HasInt256)
+      setOperationAction(ISD::USUBSAT, MVT::v8i32, Custom);
 
----------------
RKSimon wrote:
> LowerADDSAT_SUBSAT supports AVX1 splitting, so why not:
> ```
> setOperationAction(ISD::USUBSAT, MVT::v8i32, HasInt256 ? Legal : Custom);
> ```
> 
> Also, v4i64  should be ok as well.
I've added this to get the splitting, but I don't think v8i32 would be legal, as we need it to expand. Maybe `hasInt256 ? Expand : Custom` would make it more obvious what the intention here is?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56636