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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Thank you for the review @andwar !

Francesco



================
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.");
----------------
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.


================
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))
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76209