[PATCH] D113489: [AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR
Peter Waller via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 9 08:44:43 PST 2021
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
More information about the llvm-commits
mailing list