[PATCH] D75580: [llvm][CodeGen][SVE] Implement IR intrinsics for gather prefetch.

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added a comment.

Hi @fpetrogalli, thank you for working on this!

My main points:

- ACLE allows scalar indices that may be out of range for `PRFH <prfop>, <Pg>, [<Zn>.S{, #<imm>}]`. Will we be able to cater for those with your approach?
- Could you please remove the extra empty lines from test files? (and make sure that the formatting is consistent)
- We tend to put declarations at the end of the test files (I've not seen a test with a different style, but I've not seen them all either ;-) )



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1258
+
+class SVE_prf_scaled
+    : Intrinsic<[],
----------------
This is an unwritten rule, but so far class definitions and intrinsics definitions are kept separately (i.e. all class definitions first, followed by all intrinsics definitions). Some classes are re-used by many intrinsics, so the 2 definitions (class and intrinsics) can end up being far apart anyway.

For consistency sake I'd separate the two.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1261
+                [
+                  llvm_anyptr_ty,    // Base address
+                  llvm_anyvector_ty, // offsets
----------------
[nit] I think that this way would be a bit more consistent (one space before the comment)
```
class SVE_prf_scaled
    : Intrinsic<[],
                [
                  llvm_anyptr_ty, // Base address
                  llvm_anyvector_ty, // offsets
                  LLVMScalarOrSameVectorWidth<1, llvm_i1_ty>, // Predicate
                  llvm_i32_ty // prfop
                ],
                [IntrArgMemOnly, NoCapture<0>, ImmArg<3>]>;
```


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1275
+                [
+                  llvm_anyvector_ty,    // Base address
+                  llvm_i64_ty, // immediate offset
----------------
[nit] Base addresses


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:13
 
+class gather_prf_scaled_pat_frag<SDPatternOperator intrinsic, code predicate>
+  : PatFrag<(ops node:$base, node:$vec_offset, node:$pred, node:$prfop),
----------------
Please, could this block) of code (and it's sunblocks) could be documented/commented consistently with the rest of the file? E.g.:
```
// Prefetches - pattern definitions
//
```
Having said that, this file is growing organically so we may want to revisit the current style.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:18
+
+ def gather_prfb_scaled_uxtw
+   : gather_prf_scaled_pat_frag<int_aarch64_sve_gather_prf_scaled_uxtw,
----------------
Why extra indentation?


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:40
+   : gather_prf_scaled_pat_frag<int_aarch64_sve_gather_prf_scaled_uxtw,
+   [{ return cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::nxv4i16 || cast<MemIntrinsicSDNode>(N)->getMemoryVT() == MVT::nxv4f16; }]>;
+
----------------
sdesmalen wrote:
> nit: format to split statement on two lines?
IMO this would be easier to read if it was split across two lines.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-gather-prefetches-scaled-offset.ll:262
+
+
+
----------------
[nit] many empty lines


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-gather-prefetches-vect-base-imm-offset.ll:10
+; CHECK-NEXT:  ret
+  call void @llvm.aarch64.sve.gather.prfb.nx4vi32(<vscale x 4 x i32> %bases, i64 7, <vscale x 4 x i1> %Pg, i32 1)
+  ret void
----------------
What about indices that are out of range for `<prfop>, <Pg>, [<Zn>.S{, #<imm>}]`?  IIUC, this is similar to what's tested here:

https://github.com/llvm/llvm-project/blob/e60c28746b0cf30323e736f34048b02ff34688f6/llvm/test/CodeGen/AArch64/sve-intrinsics-gather-loads-vector-base-imm-offset.ll#L284

Currently this is neither supported nor tested.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-gather-prefetches-vect-base-imm-offset.ll:36
+
+
+; PRFH <prfop>, <Pg>, [<Zn>.D{, #<imm>}] -> 64-bit element
----------------
[nit] empty line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75580