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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice one! Looks very useful. I _think_ bigendian should be fine here, so long as we don't use the wrong type.

There are a lot of other masked load tests in mve-masked-load.ll. I think we should add the same for widening loads and narrowing stores. There should be tests for things like align1 and different passthru values. We might want extra tests for odd types too, if we are making them legal through isLegalMaskedLoad.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13760
 
+static SDValue CombineExtendingMaskedLoad(SDNode *Ext, SelectionDAG &DAG) {
+  MaskedLoadSDNode *Ld = dyn_cast<MaskedLoadSDNode>(Ext->getOperand(0));
----------------
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.


================
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), [{
----------------
I think these (and perhaps the ones above, tbh) should maybe need "let ScalarMemoryVT = i8;". To ensure they are extending from the correct types?


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:494
 
 bool ARMTTIImpl::isLegalMaskedLoad(Type *DataTy) {
   if (!EnableMaskedLoadStores || !ST->hasMVEIntegerOps())
----------------
I think changes here might mean we need to handle stores too, or (temporarily) split the two out from one another.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:506
+    // sext/zext integers.
+    if (VecWidth > 128)
       return false;
----------------
What does this mean for a v2i32, or other weird types (for us)?


================
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
----------------
This looks odd to me, with the vpsel. There is legalising code in LowerMLOAD, which might be doing something wrong.


================
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
----------------
This is wrong at the moment? Same for all the other masked stores?


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

https://reviews.llvm.org/D68337