[PATCH] D155218: [InstCombine] Optimize addition/subtraction operations of splats of vscale multiplied by a constant

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1587
+  auto m_ConstMultipliedVscale =
+      m_CombineOr(m_CombineOr(m_VScale(), m_c_Mul(m_Constant(), m_VScale())),
+                  m_Shl(m_VScale(), m_Constant()));
----------------
It's worth verifying but I don't think you need the commutative version here because constants should always be canonicalised to the RHS.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1591
+  // Splat of the expression from above
+  auto m_SplatVscale =
+      m_Shuffle(m_InsertElt(m_Value(), m_ConstMultipliedVscale, m_ZeroInt()),
----------------
If I'm nit picking I think you should use `VScale` consistently and not `Vscale`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1599-1602
+  if (match(&Inst,
+            m_c_Add(m_c_Add(m_Value(A), m_SplatVscale), m_SplatVscale)) &&
+      match(&Inst, m_c_Add(m_c_Add(m_Specific(A), m_Instruction(SplatB)),
+                           m_Instruction(SplatC)))) {
----------------
This seems wasteful given the number of times you're matching `m_SplatVscale`.  Perhaps drop the first `match` from all the `if` blocks and then once you've figured out the add/sub combo you can have a single block that does:
```
if (!match(SplatB, m_SplatVscale) || !match(SplatC, m_SplatVscale))
  return nullptr;
```

Perhaps you don't even need `m_SplatVscale` because `getSplatValue` returns null on failure so the check could be:
```
if (!B || !match(B, m_ConstMultipliedVscale) ||
    !C || !match(C, m_ConstMultipliedVscale))
  return nullptr;
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1653-1654
 
+  if (auto *I = foldVScaleSplatAddSub(Inst))
+    return I;
+
----------------
I still think this is wasteful as we're going to run through several redundant match calls before eventually hitting the null return for every bin op other than Add and Sub.  To me it seems better to call this directly within `visitAdd` and `visitSub`.  You'll see there's related precedent here with `foldBinOpShiftWithShift` amongst others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155218