[PATCH] D68337: [ARM][MVE] Enable extending masked loads

[Sam Parker via Phabricator via llvm-commits]

samparker marked 4 inline comments as done.
samparker added a comment.

Thanks for those points, I'll add loads more tests.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13760
 
+static SDValue CombineExtendingMaskedLoad(SDNode *Ext, SelectionDAG &DAG) {
+  MaskedLoadSDNode *Ld = dyn_cast<MaskedLoadSDNode>(Ext->getOperand(0));
----------------
dmgreen wrote:
> How come this isn't in target independent code? I would expect this combine not to be MVE specific, so long as it's legal. I'm not sure if there are ways currently to check if a "widening masked load" is legal or not, in the same way as there are for normal loads.
good point.


================
Comment at: lib/Target/ARM/ARMInstrMVE.td:5019
 }]>;
+def sextmaskedload8 : PatFrag<(ops node:$ptr, node:$pred, node:$passthru),
+                              (masked_ld node:$ptr, node:$pred, node:$passthru), [{
----------------
dmgreen wrote:
> I think these (and perhaps the ones above, tbh) should maybe need "let ScalarMemoryVT = i8;". To ensure they are extending from the correct types?
I'll give it a go.


================
Comment at: test/CodeGen/Thumb2/mve-masked-ldst.ll:112
+; CHECK-LE-NEXT:    vldrbt.s16 q1, [r2]
+; CHECK-LE-NEXT:    vpsel q0, q1, q0
+; CHECK-LE-NEXT:    vpst
----------------
dmgreen wrote:
> This looks odd to me, with the vpsel. There is legalising code in LowerMLOAD, which might be doing something wrong.
Is the vpsel not just handling the predicate on the store?


================
Comment at: test/CodeGen/Thumb2/mve-masked-ldst.ll:191
+; CHECK-LE-NEXT:    vldrht.u16 q0, [r2]
+; CHECK-LE-NEXT:    vstrbt.8 q0, [r0]
+; CHECK-LE-NEXT:    bx lr
----------------
dmgreen wrote:
> This is wrong at the moment? Same for all the other masked stores?
Yes? I hadn't looked at stores but it looks like these should now be vstrb.16.


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

https://reviews.llvm.org/D68337