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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

A couple of minor requests but otherwise looks good.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1614
+  // 2) A multiplication of a constant and VScale
+  // 3) A shift left of VScale on a constant value
+  auto m_ConstMultipliedVScale =
----------------
by


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1656-1657
+
+  B = getSplatValue(SplatB);
+  C = getSplatValue(SplatC);
+
----------------
Better to move the initialisation here (i.e. `Value *B = `) since there's no other uses.


================
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)))) {
----------------
igor.kirillov wrote:
> paulwalker-arm wrote:
> > 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;
> > ```
> What do you think about this version? My first version was using `getSplatValue`, but then I had to decide the exact order of A, B and C. Using the current approach, we can first match A, SpatB, SplatC and then match the exact add/sub operations without calling splat matcher several times.
Seems like a reasonable compromise to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155218