[PATCH] D94615: [RISCV] Add RVV insertelt/extractelt scalable-vector patterns

Fraser Cormack via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 25 04:56:12 PST 2021


frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1323
+  SDValue SplattedIdx =
+      DAG.getSplatVector(VecVT, DL, DAG.getSExtOrTrunc(Idx, DL, MVT::i64));
+
----------------
frasercrmck wrote:
> craig.topper wrote:
> > Index isn't signed so this should probably be ZExtOrTrunc?
> Makes sense. I had it SExtOrTrunc because it should be correct (?)  and it was easier to identify as a sign-extended 32-bit value in the RV32 SPLAT_VECTOR legalization. With ZExtOrTrunc the codegen is currently worse.
> 
> Though I think I need to take another look I think this function might be doing splats wrong, and/or creating illegal values. I'll take a look on Monday.
I've replaced this by directly using `SPLAT_VECTOR_I64` to splat the index.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:605
+
+    def : Pat<(vti.Vector (insertelt_node (vti.Vector vti.RegClass:$merge),
+                                          vti.ScalarRegClass:$rs1, uimm5:$rs2)),
----------------
frasercrmck wrote:
> craig.topper wrote:
> > This feels like a pretty complicated output sequence. Should we custom lower it?
> Custom-lowered to a `(slideup (vmv (slidedown)))`? Or even a `(slideup (insertelt (slidedown), val, 0))` ?Might be a good idea given the we need slidedown for RV32 legalization anyway.
I've updated the patch to always lower insert/extract to slides and an insert/extract of the first element. The patterns are more manageable as a result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94615



More information about the llvm-commits mailing list