[PATCH] D147713: [RISCV] Combine concat_vectors of loads into strided loads

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Generally looks pretty good, a couple small comments.  As always, I'd like to wait for @craig.topper's feedback as he knows this part of things much better than I do.

@craig.topper To anticipate one of your objections, this does need to be a DAG combine not IR.  The test cases here are IR based, but the original case I saw this (and told Luke) was due to type legalization in DAG.  That particular case is now fixed by a costing change, but I suspect we have other such cases.

For follow up, a couple ideas:

1. We don't need to match a full concat_vector.  Any adjacent two element pair is profitable to fold.  As a result, we can allow concat_vectors with unrelated operands.

2. There's an inverse form of this for extract_subvector+store.  That one isn't as straight forward to match, and isn't currently being emitted by SLP (or other known source).  Given that, I'd suggest deferring for now.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11437
+        DAG.getMachineFunction().getMachineMemOperand(
+            BaseLd->getMemOperand(), 0, WideVecVT.getStoreSize()));
+    DAG.makeEquivalentMemoryOrdering(BaseLd, StridedLoad);
----------------
The size on this memory operand is wrong.  The memory region accessed isn't the size of the result vector, it's the entire stride region which can be much larger.  You can probably just use an unknown size here - unless we already have the utility code for this somewhere else. 


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-combine.ll:126
 ; Vector is too large to fit into a single strided load
 define void @strided_constant_v4i32(ptr %x, ptr %z) {
 ; CHECK-LABEL: strided_constant_v4i32:
----------------
The comment and the code appear out of sync here.  I think the code is correct.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-combine.ll:237
   ret void
 }
----------------
Can you add a couple of negative tests?

1) A case where the stride is not equal.  (i.e. we recognize stride mismatch.)

2) A case where the resulting type is not legal.

3) A case with a non-simple load.  

4) A case where one of the operands is not a load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147713