[PATCH] D112078: [AArch64][SVE] Add new ld<n> intrinsics that return a struct of vscale types

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Hi @bsmith 
I was not added to the review so you can ignore my comments.
But if you want to increase a fellow contributor knowledge in LLVM I posted some questions.



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:977
+
+  class AdvSIMD_4Vec_PredLoad_Intrinsic
+    : DefaultAttrsIntrinsic<[llvm_anyvector_ty, LLVMMatchType<0>, LLVMMatchType<0>, LLVMMatchType<0>],
----------------
Should this be align? Like it is in class AdvSIMD_4Vec_PredStore_Intrinsic.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1504
 
-  SDValue Ops[] = {N->getOperand(1), // Predicate
-                   Base,             // Memory operand
+  SDValue Ops[] = {N->getOperand(IsIntr ? 2 : 1), // Predicate
+                   Base,                          // Memory operand
----------------

I would be tempted to check if call N->getOperand() is what is expected. 
But I believe once we are here these checks don't need to happen anymore. Correct? 
I am just saying this because the operand's selection is according to a newly introduced parameter that I believe is asking if this is Intrinsic.
I believe before you had tablegen to help to check the types, but now you are using this function for other intrinsic.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3904
+      } else if (VT == MVT::nxv8i16 || VT == MVT::nxv8f16 ||
+                 (VT == MVT::nxv8bf16 && Subtarget->hasBF16())) {
+        SelectPredicatedLoad(Node, 2, 1, AArch64::LD2H_IMM, AArch64::LD2H,
----------------
Just in case:
When I had to work with ld3 and make tests for it.
I needed  to change the ld to accept these types:
    2f16, 2b16
    4f16, 4bf16  and
    2f32 
Should these types be here too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112078