[PATCH] D76209: [llvm][SVE] Addressing mode for FF/NF loads.

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4488
+/// integers MVT::nx<M>xi<bits> such that M x bits = 128.
+static EVT getSVEPackedVectorTypeFromEC(LLVMContext &Ctx, ElementCount EC) {
+  assert(EC.Scalable && "Expecting scalable Element Count.");
----------------
fpetrogalli wrote:
> andwar wrote:
> > Is this change needed here?
> I wouldn't say needed, but:
> 
> 1. as far as I know, all types handled by this component in LLVM are legal: which meas that it is very unlikely that we will ever enter the brnach code following. If we ever do enter this branch, it means that something elase is broken before in the pipeline. Addressing mode decomposition doesn't seem to me the right place to catch it.
> 
> ```
>   if (NumElts != 2 && NumElts != 4 && NumElts != 8 && NumElts != 16)
>     return EVT();
> ```
> 
> 2. What this method does is "give me a scalable vector of N packed lanes of integers". Whether the "N" comes of a vector of `i1` lanes, it doesn't matter.  Hence, I think it is better to use ElementCount in input, not a vector EVT of `i1`lanes. So, I think we can also remove the first if condition.
> 
> All in all, I think it makes sense to make the changes you see in this method.
Thank you for the explanation, it makes sense! 

* Still, these changes are unrelated to what this patch is for. I think that this sort of refactoring should happen in a separate patch. If you want to keep it here, could you please list the NFC changes in the commit message?
* I think that with these changes this method can be replaced with `getSVEContainerType`. 




================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4498
 /// Root. If such EVT cannot be retrived, it returns an invalid EVT.
 static EVT getMemVTFromNode(LLVMContext &Ctx, SDNode *Root) {
   if (isa<MemSDNode>(Root))
----------------
fpetrogalli wrote:
> andwar wrote:
> > Why do you have to update this method? I've tried to find the relation with other changes in this file and I failed :)
> The method `SelectAddrModeIndexedSVE` operates on the "memory VT" of the underlying SDNode. One it determines it, it uses it to computed the decomposition of the ADD into a Base and an integer `MUL VL` Offset.
> 
> I had to modify this method because it needs to learn how to extract the MemVT from the SDNodes that represent the LDNF/LDFF. I then rearranged the code for the PRF intrinsic because (to me) it made mode sense to handle all cases under the same switch.
> 
> It would be nice if we could use the same method to retrieve the memory VT for all the SVE LD intrinsic, but for the custom ISD nodes there is not such thing as  `getMemoryVT()`, like there is for the `MemSDNode` class.
> 
> We could argue to use `MemIntrinsicSDNode` (which have the `getMemoryVT` method), but at this point in the lowering sequence the `AArch64ISD` nodes have replaced the input `Intrinsic::aarch64_sve_ld...` nodes, so I cannot cast the nodes into instances of `MemIntrinsicSDNode`.
OK, now I understand, thanks! I think that it would be very helpful to explain this in a comment somewhere within the method. In particular, **why** does this method differentiate between custom ISD nodes and intrinsics? Otherwise the code is a bit counterintuitive.

It's unfortunate that such a small method requires nested `switch` statements.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4517
 
-  // We are using an SVE prefetch intrinsic. Type must be inferred
-  // from the width of the predicate.
-  return getPackedVectorTypeFromPredicateType(
-      Ctx, Root->getOperand(2)->getValueType(0));
+  llvm_unreachable("Non SDNode-derived classes should be "
+                   "handled inside the switch statement.");
----------------
This `llvm_unreachable` is a bit confusing here (relates to the nested `switch`). Wouldn't it make more sense to have a `default` case within the nested `switch` block?


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-loads-nf.ll:18
+; CHECK: rdvl    x[[OFFSET:[0-9]+]], #-9
+; CHECK: add     x[[BASE:[0-9]+]], x0, x[[OFFSET]]
+; CHECK: ldnf1b { z0.b }, p0/z, [x[[BASE]]]
----------------
`CHECK-NEXT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76209