[PATCH] D106549: [AArch64][SVE] Combine bitcasts to predicate types with vector inserts of loads

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:286
+bool SVEIntrinsicOpts::optimizePredicateStore(Instruction *I) {
+  auto *F = I->getParent()->getParent();
+  auto Attr = F->getFnAttribute(Attribute::VScaleRange);
----------------
Not that bothered but you could pass this into the function given optimizeInstructions already knows it.  If you do keep the lookup then can you use `I->getFunction()`.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:304
+  auto *Store = dyn_cast<StoreInst>(I);
+  if (!Store)
+    return false;
----------------
To keep things simple (pun intended) I suggest adding ` || !Store->isSimple())` so that we only transform ordinary stores.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:308
+  // ..that is storing a predicate vector sized worth of bits..
+  if (dyn_cast<FixedVectorType>(Store->getOperand(0)->getType()) !=
+      FixedPredType)
----------------
Is the `dyn_cast` necessary?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:352
+bool SVEIntrinsicOpts::optimizePredicateLoad(Instruction *I) {
+  auto *F = I->getParent()->getParent();
+  auto Attr = F->getFnAttribute(Attribute::VScaleRange);
----------------
As with optimizePredicateStore.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:368-369
+
+  if (I->use_empty())
+    return false;
+
----------------
Although possible, is this worth caring about?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:389
+  auto *Load = dyn_cast<LoadInst>(IntrI->getOperand(1));
+  if (!Load)
+    return false;
----------------
As with the store case, we should only allow ordinary loads.


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:393
+  // ..that is loading a predicate vector sized worth of bits..
+  if (dyn_cast<FixedVectorType>(Load->getType()) != FixedPredType)
+    return false;
----------------
Is the dyn_cast necessary?


================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:397
+  IRBuilder<> Builder(I->getContext());
+  Builder.SetInsertPoint(I);
+
----------------
The insertion point needs to be where the original load is to ensure the new load maintains the same load/store ordering.


================
Comment at: llvm/test/CodeGen/AArch64/sve-insert-vector-to-predicate-load.ll:10
+; CHECK-NEXT:    ret <vscale x 16 x i1> [[TMP2]]
+  %load = load <2 x i8>, <2 x i8>* %addr, align 4
+  %insert = tail call <vscale x 2 x i8> @llvm.experimental.vector.insert.nxv2i8.v2i8(<vscale x 2 x i8> undef, <2 x i8> %load, i64 0)
----------------
Given the new load placement issue I mentioned above it's worth having the load in a separate basic block in order to validate that fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106549