[PATCH] D103912: LoadStoreVectorizer: support different operand orders in the add sequence match

[Volkan Keles via Phabricator via llvm-commits]

volkan added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:391
+         (!Signed && BinOpI->hasNoUnsignedWrap());
+}
+static bool checkIfSafeAddSequence(const APInt &IdxDiff, Instruction *AddOpA,
----------------
Nit: Add empty line?


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:393
+static bool checkIfSafeAddSequence(const APInt &IdxDiff, Instruction *AddOpA,
+                                   unsigned MatchinOperandA,
+                                   Instruction *AddOpB,
----------------
You can rename `MatchinOperand[AB]` as `MatchinOpIdx[AB]` or `MatchinOperandIdx[AB]` to make it clear that this is an index.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:415
+         checkNoWrapFlags(AddOpA, Signed) && checkNoWrapFlags(AddOpB, Signed));
+  unsigned OtherOperandA = 1 - MatchinOperandA;
+  unsigned OtherOperandB = 1 - MatchinOperandB;
----------------
You can move this into the if statement below as there is no other users and get the Value directly as below:
```
Value *OtherOpA = AddOpA->getOperand(MatchinOpIdxA == 0 ? 1 : 0);
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:420
+      AddOpB->getOperand(MatchinOperandB)) {
+    Value *RHSA = AddOpA->getOperand(OtherOperandA);
+    Value *RHSB = AddOpB->getOperand(OtherOperandB);
----------------
This might be LHS or RHS based on `MatchinOperand[AB]`, could you rename these variables?


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:430
+      if (OpRHSB->getOperand(0) == RHSA && IdxDiff.getSExtValue() == CstVal)
+        Safe = true;
+    }
----------------
No need to check other cases, you can return `true` directly and get rid of `Safe`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:523
+  // Second attempt: check if we have eligible add NSW/NUW instruction
+  // sequinces.
   OpA = dyn_cast<Instruction>(ValA);
----------------
Typo


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

https://reviews.llvm.org/D103912