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

[Igor Kirillov via Phabricator via llvm-commits]

igor.kirillov marked 2 inline comments as done.
igor.kirillov added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1529
+  // where B and C are splats of VScale multiplied by a number
+  if (Opcode == Instruction::Add || Opcode == Instruction::Sub) {
+    Value *A, *B, *C;
----------------
goldstein.w.n wrote:
> paulwalker-arm wrote:
> > This function seems to handle cases where the opcode is not relevant.  However, you only care about two specific opcodes so this doesn't looks like the correct resting place for this code.  Placing it somewhere more specific to `add` and `sub` might allow you to simplify the logic.
> Although you could imagine extending this function for any assosiative binop, i.e xor, or, and, mul...
> Probably cleaner would to be split this to a helper function so that if you don't have supported opcodes you cna just return `nullptr`. Then there won't be so much nesting.
> This function seems to handle cases where the opcode is not relevant.  However, you only care about two specific opcodes, so this doesn't looks like the correct resting place for this code.  Placing it somewhere more specific to `add` and `sub` might allow you to simplify the logic.

@paulwalker-arm, Yes, that was my first idea, but I took a look at InstCombineAddSub.cpp, and it has a minimal amount of code that is vector specific, so I decided to put the logic into foldVectorBinop.

@goldstein.w.n, Put everything into a separate function. I am unsure LoopVectorize could generate such code (with xor etc.). 


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1534
+    if (!C || !isConstantMultipliedVScale(C)) {
+      C = getSplatValue(RHS);
+      Nested = dyn_cast<Instruction>(RHS);
----------------
paulwalker-arm wrote:
> Should this be `C = getSplatValue(LHS);`? if so then perhaps this highlights some missing tests?
Yes, indeed! But the algorithm now is completely different.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1550-1551
+          // Deduce the new opcode based on the positivity of splat operations
+          auto IsBPositive = Opcode2 == Instruction::Add;
+          auto IsCPositive = Opcode == Instruction::Add;
+          Instruction::BinaryOps NewOpcode1;
----------------
paulwalker-arm wrote:
> Not sure this naming is correct because the opcode says nothing about the signedness of the data.
No more signedness


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155218