[PATCH] D44785: Lowering x86 adds/addus/subs/subus intrinsics (llvm part)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

I've been trying to see if I can spot anything that would expalin the problem here, but I don't see anything so far. I didn't find some other things I missed during the original review.



================
Comment at: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp:36006
+                                      const SDLoc &DL) {
+  if (!VT.isVector() || !VT.isSimple())
+    return SDValue();
----------------
Is this isSimple check needed? It limits the maximum vector width we can recognize to 2048 bits, but if someone ever changes the largest vXi16/vXi8 type in MachineValueType.h due to some other target in the future this would change the behavior of this code.


================
Comment at: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp:36056
+  // one of them is and the other one is constant.
+  unsigned ExtendOpcode = Signed ? ISD::SIGN_EXTEND :
+                                   ISD::ZERO_EXTEND;
----------------
If the middle end can determine the sign bit of the input to the extend to be 0, it might replace the sext with a zext. Should we be using computeNumSignbits/MaskedValueIsZero here instead of checking specific opcodes?


================
Comment at: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp:36065
+  } else if (LHSOpcode == ExtendOpcode &&
+             ISD::isBuildVectorOfConstantSDNodes(RHS.getNode())) {
+    LHS = LHS.getOperand(0);
----------------
Don't we need to verify the constant has the right number of sign bits or zero bits?


Repository:
  rL LLVM

https://reviews.llvm.org/D44785