[PATCH] D20931: [X86] Reduce the width of multiplification when its operands are extended from i8 or i16

[Wei Mi via llvm-commits]

wmi added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26446
@@ +26445,3 @@
+  bool IsPositive[2] = {false, false};
+  for (unsigned i = 0, e = N->getNumOperands(); i < e; i++) {
+    SDValue Opd = N->getOperand(i);
----------------
mkuper wrote:
> Nit - it's a bit misleading to have N->getNumOperands() here, and then index into an array of size 2.
> Maybe assert N->getNumOperands() == 2 (or just use 2 here, but that's less self-documenting, I guess).
That is misleading indeed. Fixed and Added an assertion.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26470
@@ +26469,3 @@
+          continue;
+        auto *CN = dyn_cast<ConstantSDNode>(SubOp);
+        if (!CN)
----------------
mkuper wrote:
> Perhaps use ComputeNumSignBits on the BUILD_VECTOR elements as well, instead of checking for const/undef?
> Although I'm really not sure if that gains anything in practice, and it won't make the code significantly smaller, so it's may not be worth the overhead.
ComputeNumSignBits doesn't work for BUILD_VECTOR for now. It always return 1.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26480
@@ +26479,3 @@
+      SignBits[i] = DAG.ComputeNumSignBits(Opd);
+      if (Opd.getOpcode() == ISD::ZERO_EXTEND)
+        IsPositive[i] = true;
----------------
mkuper wrote:
> Maybe computeKnownBits instead of special-casing ZERO_EXTEND?
> Although, the same as above applies, not at all sure it's worth the overhead.
The code seems longer and costlier that way, so I choose to special-case ZERO_EXTEND here.


Repository:
  rL LLVM

http://reviews.llvm.org/D20931