[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

I've had a brief look, some typographical comments.

For some reason the patch has failed to apply and the tests have failed to run in CI, perhaps you could try a rebase and rerun arc diff to see if that fixes it?



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:701
+                                                       const DataLayout &DL) {
+  Type *VecOp = II.getType();
+  Value *PointerOp = II.getOperand(1);
----------------
Conventionally, I think this would be called `VecTy` rather than `VecOp`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:702
+  Type *VecOp = II.getType();
+  Value *PointerOp = II.getOperand(1);
+
----------------
I've seen more uses of `Ptr` than "Pointer".


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+  Builder.SetInsertPoint(&II);
+  auto VScalePointer =
+      Builder.CreateBitCast(II.getOperand(1), II.getType()->getPointerTo());
----------------
The name VScale brings up an association with "The runtime value of VScale", and this is not a pointer to it, so I think a name more in keeping with the codebase would be something like `VecPtr`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:733
+                                                       const DataLayout &DL) {
+  auto VecOp = II.getOperand(0);
+  auto PointerOp = II.getOperand(2);
----------------
Inconsistent use of `auto` (Value* was used above, which I think the coding guide would prefer).


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:734
+  auto VecOp = II.getOperand(0);
+  auto PointerOp = II.getOperand(2);
+
----------------
You've named getOperand(0) and getOperand(2) here, I think for consistency/completeness you may as well name the final operand, even though it's only used once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489