[PATCH] D136015: [InstCombine] Fold series of instructions into mull

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1270
 
+/// Tries to simplify a few sequence operations into MULL
+static Instruction *SimplifyMull(BinaryOperator &I) {
----------------
Add a better description for the full transform. Something like:
  /// Reduce a sequence of masked half-width multiplies to a single multiply.
  /// ((XLow * YHigh) + (YLow * XHigh)) << HalfBits) + (XLow * YLow) --> X * Y



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1271
+/// Tries to simplify a few sequence operations into MULL
+static Instruction *SimplifyMull(BinaryOperator &I) {
+  if (!I.getType()->isIntegerTy())
----------------
Function names should start with lower-case letter. "Simplify" has a distinct meaning in LLVM combining - it suggests that we are not creating a new instruction. Even though it is misused in other places including in this file, we shouldn't do that again. 

I suggest naming this "foldLongMultiply" or "foldBoxMultiply" ( https://www.ixl.com/math/grade-4/box-multiplication ) or something like that, so it's more obvious that we are reducing a sequence of mul and add to something else.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1275
+
+  unsigned BitWidth = I.getType()->getIntegerBitWidth();
+  // Skip the odd bitwidth types and large bitwidth types
----------------
I don't see a reason to exclude vectors from this transform. Just change this line?
  unsigned BitWidth = I.getType()->getScalarSizeInBits();



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1277
+  // Skip the odd bitwidth types and large bitwidth types
+  if ((BitWidth & 0x1) || (BitWidth > 128))
+    return nullptr;
----------------
Similarly, why exclude wide widths? We're already using APInt::getMaxValue(), so just use that APInt in the m_SpecificInt() calls?


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

https://reviews.llvm.org/D136015