[PATCH] D46179: [X86] Lowering adds/addus/subs/subus intrinsics to native IR (LLVM part)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36147
+      }
+    // Build vector of constant nodes. Each of them needs to be a correct
+    // extension from a constant of ScalarVT type.
----------------
Move this comment into the if body so it doesn't interfere with the closing curly brace from the above 'if'


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36153
+        APInt Elt = cast<ConstantSDNode>(Op.getOperand(i))->getAPIntValue();
+        Elt = Elt.getHiBits(Signed ? ExtPartSize + 1 : ExtPartSize);
+        if ((Signed && (!Elt.isAllOnesValue() && !Elt.isNullValue())) ||
----------------
Can you use APInt::isSignedIntN/isIntN here?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36177
+  } else {
+    LHS = LHS.getOperand(0);
+    RHS = RHS.getOperand(0);
----------------
What if the SIGN_EXTEND/ZERO_EXTEND was from an even smaller type? You can't just remove it. You need to narrow it. Always emitting an ISD::TRUNCATE may be sufficient. DAG combine should be able to simplify it.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:36183
+  // as those before extension.
+  if (LHS.getValueType() != VT || RHS.getValueType() != VT)
+    return SDValue();
----------------
Oh I see, you covered for that case here. But do we really need to do this? Can we just make smaller extends?


Repository:
  rL LLVM

https://reviews.llvm.org/D46179