[PATCH] D71698: [AArch64][SVE] Add intrinsic for non-faulting loads

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9998
+
   // GLD1* instructions perform an implicit zero-extend, which makes them
   // perfect candidates for combining.
----------------
Could you replace `GLD1*` with `Load`? I believe that that will be still correct with the added bonus of covering the new case :)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11051
+  if (ContainerVT.isInteger()) {
+    switch (VT.getVectorNumElements()) {
+    default: return SDValue();
----------------
You could use `getSVEContainterType` here instead. You'll need to extend it a wee bit.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12284
 
   // Gather load nodes (e.g. AArch64ISD::GLD1) are straightforward candidates
   // for DAG Combine with SIGN_EXTEND_INREG. Bail out for all other nodes.
----------------
The following `switch` statement will now cover more than just *Gather* nodes. Maybe `SVE load nodes` instead? 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12328-12331
+  Ops.push_back(Src->getOperand(0));
+  Ops.push_back(Src->getOperand(1));
+  Ops.push_back(Src->getOperand(2));
+  Ops.push_back(Src->getOperand(3));
----------------
Why not:
```
SmallVector<SDvalue, 4> Ops = {Src->getOperand(0), Src->getOperand(1), Src->getOperand(2), Src->getOperand(3), Src->getOperand(4)};
```
?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12332
+  Ops.push_back(Src->getOperand(3));
+  if (NewOpc != AArch64ISD::LDNF1S)
+    Ops.push_back(Src->getOperand(4));
----------------
Could you add a comment explaining what the underlying difference between `LDNF1S` and `GLD1S` is? Otherwise it's not clear why this `if` statement is needed. IIUC, `GLD1S` has an extra argument for the offsets (hence 5 args vs 4).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71698