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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 6 inline comments as done.
fpetrogalli added a comment.

@andwar , thank you for the review.

I'll update the patch asap.

> 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.

      

> [nit] AFAIK, + is never used in file names (e.g. sve-pred-non-temporal-ldst-addressing-mode-reg+reg.ll). I couldn't find any specific rule for that though.

Fair point, I'll remove the plus.

> [nit] The idea of %sv2i1  = type <vscale x 2 x i1> is very neat, but I'm slightly worried that that will complicate searching through test files (we are quite familiar with <vscale x 2 x i1>, not so much with %sv2i1, so you might miss cases when using grep).

Very good point. The type redefinition saved me a lot of typing, but now that the tests are there it is better o remove it.

>   Are the DAG Combine rules required for this patch? If so, could you add tests for them?

They are necessary for the reg+imm tests. They are needed to make sure that the ADD node of the base address is always in the format (ADD %BASE (VSCALE CONST)). I'll see if I can add some unit tests specifically for the combiner changes.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4439
+    if ((Offset >= Min) && (Offset <= Max)) {
+      Base = N.getOperand(0);
+      OffImm = CurDAG->getTargetConstant(Offset, SDLoc(N), MVT::i64);
----------------
andwar wrote:
> `Base` and `OffImm` are only only needed when building in Debug mode and running e.g. `llc` with `-debug`.  Maybe move that inside `LLVM_DEBUG`? 
No, they are output parameters, passed by reference. They are not just for debug purpose.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4441
+      OffImm = CurDAG->getTargetConstant(Offset, SDLoc(N), MVT::i64);
+      LLVM_DEBUG(dbgs() << "Match found:\n"; dbgs() << "ROOT:\n"; Root->dumpr();
+                 dbgs() << "BASE:\n"; Base.dumpr(); dbgs() << "OFFSET:\n";
----------------
andwar wrote:
> [nit] `Match found` is very generic (and you use it twice). Maybe sth more specific?
I will remove these debug messages, they are not very helpful.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1304
                        (RegImmInst ZPR:$val, (PTrue 31), GPR64sp:$base, simm4s1:$offset)>;
+    let AddedComplexity = 2 in {
+      def _reg_imm : Pat<(store(Ty ZPR:$val),  (am_sve_indexed_s4 GPR64sp:$base, simm4s1:$offset)),
----------------
This is to be removed.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1321
                   (RegImmInst (PTrue 31), GPR64sp:$base, simm4s1:$offset)>;
+    let AddedComplexity = 2 in {
+      def _reg_imm : Pat<(Ty (load  (am_sve_indexed_s4 GPR64sp:$base, simm4s1:$offset))),
----------------
This will be removed too.


================
Comment at: llvm/test/CodeGen/AArch64/sve-gep.ll:7
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    rdvl x8, #1
-; CHECK-NEXT:    add x0, x0, x8, lsl #2
+; CHECK-NEXT:    rdvl x8, #4
+; CHECK-NEXT:    add x0, x0, x8
----------------
andwar wrote:
> Unrelated changes.
No, I had to fix this because of the DAG combine changes.


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

https://reviews.llvm.org/D74254