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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:728-738
+static Optional<Instruction *>
+instCombineSVELD1(InstCombiner &IC, IntrinsicInst &II, const DataLayout &DL) {
+  Value *PtrOp = II.getOperand(1);
+  Type *VecTyPtr = II.getType()->getPointerTo();
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
----------------
Does this function and instCombineSVEST1 offer any real value? I cannot see them having other uses and the implementation is largely already available in the caller.  I think it would be cleaner to just have the
```{
  LoadInst *Load = Builder.CreateLoad(II.getType(), VecPtr);
  return IC.replaceInstUsesWith(II, Load);
}
```
part inside `instCombineSVEMaskedLD1`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:769
+  Value *VecPtr = Builder.CreateBitCast(PtrOp, VecTyPtr);
+  (void)Builder.CreateStore(VecOp, VecPtr);
+  return IC.eraseInstFromFunction(II);
----------------
Is this necessary?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:788
+  auto VecPtr = Builder.CreateBitCast(PtrOp, VecTyPtr);
+  (void)Builder.CreateMaskedStore(VecOp, VecPtr, PtrOp->getPointerAlignment(DL),
+                                  Pred);
----------------
Is this necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113489