[PATCH] D67005: [ARM] MVE: isLegalMaskedLoad

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

It's a shame that the isLegalMaskedLoad works like that! Being given integer types. MVE only has some expands and we don't handle all of them.

Unfortunately I don't think it's quite this simple. We still need to make sure that the vector types that we don't know how to handle are excluded correctly. We don't handle widening loads yet, for example (although we could and it would fix the testcase here I think), That's what the vector width line was trying to guard against, and the tests don't look correct.

Instead, can we just put the VecWidth check behind a check of isVectorTy? So if we are called from the vectorizer, we check the scalar type size and if we are called later from the legalise helper we check the full type. We may just be opening ourselves up to a lot more bugs/inefficiencies, we will have to make sure we test this well before we enable it.



================
Comment at: llvm/test/CodeGen/Thumb2/mve-masked-ldst.ll:28
+; CHECK-LE-NEXT:    vpst
+; CHECK-LE-NEXT:    vldrbt.u8 q0, [r2]
+; CHECK-LE-NEXT:    vmovlb.s8 q0, q0
----------------
This seems to be doing a 4xi32 cmp, followed by a 16xi8 load using that predicate, and then sign extending the wrong registers in place. It should be sign extending the first 4 value, not the 0,4,8,12 values. If I'm reading this correctly.

I'm surprised it's doing this exactly though. You may be finding some other bug here.


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

https://reviews.llvm.org/D67005