[PATCH] D119115: [RISCV] Improve insert_vector_elt for fixed mask registers.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

I'm sorry it took me so long to get round to this! I think it LGTM now other than questions/suggestions I've left. @craig.topper are you happy with it?



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4411
   if (VecVT.getVectorElementType() == MVT::i1) {
-    // FIXME: For now we just promote to an i8 vector and insert into that,
-    // but this is probably not optimal.
+    if (VecVT.isFixedLengthVector()) {
+      unsigned NumElts = VecVT.getVectorNumElements();
----------------
It's a bit of a shame we have to indent twice. We could do `if (VecVT.isFixedLengthVector() && VecVT.getVectorNumElements() >= 8)` which might be clearer for readers, even if we later have to take `NumElts` later. Just a style suggestion though.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4413
+      unsigned NumElts = VecVT.getVectorNumElements();
+      if (NumElts >= 8) {
+        MVT WideEltVT;
----------------
I think maybe `isPowerOf2_32(NumElts)` would be most correct here? I realise there's an assertion in one case below but I'm worried that something experimenting with differently-sized vectors may not hit this and get incorrect code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119115