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

Thu Feb 13 04:34:04 PST 2020

andwar added a comment.

In D74254#1872363 <https://reviews.llvm.org/D74254#1872363>, @fpetrogalli wrote:

> > IMHO every test functions (e.g.:test_masked_ldst_sv2i8 ) should either test contiguous load or store (i.e. only one thing at a time). That will help triaging potential bugs in the future and also would be consistent with other test files in this folder.
>
> I see your point, but the tests that use merge and store at the same time are using exactly the same addressing modes, it is not that they are using something different. So if something fails in the addressing mode of the load, it fails in the addressing mode of the store. Having them merged together saves quite some typing, and has no disadvantages in term of unit testing.

Ah, I've realised that you split the files per addressing modes. As the name of the patch would suggest :-) OK, keep it as it is.

Thank you for adding DAG diagrams - they are very helpful!

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2330

+  // Fold (add (vscale * C0), (vscale C1)) to (vscale  C0 + C1))
+  if (N0.getOpcode() == ISD::VSCALE && N1.getOpcode() == ISD::VSCALE) {
----------------
`vscale * C1` and `vscale * (C0 + C1)`?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3260

+  // canonicalize (sub X, (vscale * C)) to (add X,  (vscale * -C))
+  if (N1.getOpcode() == ISD::VSCALE) {
----------------
I don't quite understand the benefit of this transformation. The selection dag before and after are almost identical.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3601

+  // Fold (mul (vscale * C0), C1) to (vscale  C0 * C1))
+  if (N0.getOpcode() == ISD::VSCALE)
----------------
`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)
----------------
`vscale * (C0 * C1)`?

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4447
+
+bool AArch64DAGToDAGISel::SelectSVERegRegAddrMode(SDValue N, unsigned Scale,
+                                                  SDValue &Base,
----------------
Missing doxstring.

================
Comment at: llvm/test/CodeGen/AArch64/sve-pred-contiguous-ldst-addressing-mode-reg-imm.ll:11
+; CHECK: ld1d { z[[DATA:[0-9]+]].d }, p0/z, [x{{[0-9]+}}]
+; CHECK: st1d { z[[DATA]].d }, p0, [x{{[0-9]+}}]
+; CHECK: ret
----------------
`; CHECK-NEXT`? Here and in the following examples.

================
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
----------------
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!

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:6
+
+; Fold (add (vscale * C0), (vscale C1)) to (vscale  C0 + C1))
+define i64 @combine_add_vscale_i64() nounwind {
----------------
`vscale * C1`? and `vscale * (C0 + C1)`?

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:9
+; CHECK-LABEL: combine_add_vscale_i64:
+; CHECK-NEXT:  cntd  x0
+; CHECK-NEXT:  ret
----------------
Perhaps `; CHECK-NOT: add`?

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:25
+
+; Fold (mul (vscale * C0), C1) to (vscale  C0 * C1))
+define i64 @combine_mul_vscale_i64() nounwind {
----------------
* `vscale * (C0 * C1)`?
* What's `C0` and what is `C1` in this example?

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:33
+; CHECK-NEXT:  ret
+%vscale = call i64 @llvm.vscale.i64()
+ %mul = mul i64 %vscale, 3
----------------
[nit] Align with the following line (missing space)

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:38
+
+define i32 @combine_mul_vscale_i32() nounwind {
+; CHECK-LABEL: combine_mul_vscale_i32:
----------------
Is `nounwind` needed in these examples?

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:55
+; CHECK-NEXT:  asr   x8, x8, #4
+; CHECK-NEXT:  add   x0, x0, x8
+; CHECK-NEXT:  ret
----------------
Perhaps `; CHECK-NOT: sub`?

================
Comment at: llvm/test/CodeGen/AArch64/sve-vscale-combine.ll:77
+; CHECK-LABEL: combine_shl_vscale_i64:
+; CHECK-NEXT:  rdvl  x0, #4
+; CHECK-NEXT:  ret
----------------
`; CHECK-NOT: shl`?

================
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
----------------
Hm, since it's `6` here, shouldn't line 77 be `; CHECK-NEXT:  rdvl  x0, #6`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74254