[PATCH] D74254: [llvm][aarch64] SVE addressing modes.

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 21 inline comments as done.
fpetrogalli added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3260
 
+  // canonicalize (sub X, (vscale * C)) to (add X,  (vscale * -C))
+  if (N1.getOpcode() == ISD::VSCALE) {
----------------
andwar wrote:
> I don't quite understand the benefit of this transformation. The selection dag before and after are almost identical.
It is needed to avoid the having to deal with SUB when computing the addressing mode.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3601
 
+  // Fold (mul (vscale * C0), C1) to (vscale  C0 * C1))
+  if (N0.getOpcode() == ISD::VSCALE)
----------------
andwar wrote:
> `vscale * (C0 + C1)`?
Almost: `vscale * (C0 * C1)`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7730
 
+  // Fold (shl (vscale * C0), C1) to (vscale  C0 << C1))
+  if (N0.getOpcode() == ISD::VSCALE)
----------------
andwar wrote:
> `vscale * (C0 * C1)`?
Almost: `(vscale * (C0 << C1))`.


================
Comment at: llvm/test/CodeGen/AArch64/sve-pred-contiguous-ldst-addressing-mode-reg-imm.ll:16
+  %base_store = getelementptr <vscale x 2 x i64>, <vscale x 2 x i64> * %base, i64 -9
+  call void @llvm.masked.store.nxv2i64(<vscale x 2 x i64> %data, <vscale x 2 x i64>* %base_store, i32 1, <vscale x 2 x i1> %mask)
+  ret void
----------------
andwar wrote:
> Could you format this line (and similar lines elsewhere)? E.g.: https://github.com/llvm/llvm-project/blob/318d0ede572080f18d0106dbc354e11c88329a84/llvm/test/CodeGen/AArch64/sve-intrinsics-stores.ll#L11
> 
> Makes it easier to parse for humans :) And will be consistent with other files too!
I prefer keeping all parameters in one line.

The "consistency with other file" argument doesn't work, most of the tests for SVE uses the convention in this patch:

```
frapet01 at man-08:~/projects/upstream-clang/llvm-project/llvm/test/CodeGen/AArch64 (master)$ grep "@llvm.*)$" sve*.ll | grep -v "()" | wc -l
2076
frapet01 at man-08:~/projects/upstream-clang/llvm-project/llvm/test/CodeGen/AArch64 (master)$ grep "@llvm.*,$" sve*.ll | grep -v "()" | wc -l
1433
```

The result becomes even more unbalanced if you look at the totality of tests in the folder:

```
frapet01 at man-08:~/projects/upstream-clang/llvm-project/llvm/test/CodeGen/AArch64 (master)$ grep "@llvm.*)$" *.ll | grep -v "()" | wc -l
7260
frapet01 at man-08:~/projects/upstream-clang/llvm-project/llvm/test/CodeGen/AArch64 (master)$ grep "@llvm.*,$" *.ll | grep -v "()" | wc -l
1435
```

I run grep on master, @ `a062a3ed7fd82c277812d80fb83dc6f05b939a84`, _not_ on my dev branch :).


================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:38
+
+define i32 @combine_mul_vscale_i32() nounwind {
+; CHECK-LABEL: combine_mul_vscale_i32:
----------------
andwar wrote:
> Is `nounwind` needed in these examples?
Yes, to be able to use CHECK-NEXT after CHECK-LABEL


================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:80
+ %vscale = call i64 @llvm.vscale.i64()
+ %shl = shl i64 %vscale, 6
+ ret i64 %shl
----------------
andwar wrote:
> Hm, since it's `6` here, shouldn't line 77 be `; CHECK-NEXT:  rdvl  x0, #6`?

At IR level, it is %shl =  2^6 * VSCALE

At Assembly level, the output of RDVL is 2^4 * VSCALE

Hence, to compute %shl we need to multiply RDVL output by 2^2 -> #4 is correct.

Does that make sense? I have actually added it as a comment, but I have modified the code so that it produces rdvl #1.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74254