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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9283
+                                        ISD::NodeType ExtOpc) {
+  if (!TLI.isLoadExtLegal(ExtLoadType, VT, N0.getValueType()))
+    return SDValue();
----------------
Is it true that whenever you have a legal extending load, you will also have the equivalent legal extending masked load? (For MVE we do, but is that true for all archs?)

Do we need to add an extra set of flags for this? Or is isVectorLoadExtDesirable good enough to handle these cases when there is an asymmetry?


================
Comment at: lib/Target/ARM/ARMInstrMVE.td:5203
+            (v4i32 (MVE_VLDRBS32 t2addrmode_imm7<0>:$addr, (i32 1), VCCR:$pred))>;
+  def : Pat<(v4i32 (sextmaskedload16 t2addrmode_imm7<0>:$addr, VCCR:$pred,
+                    (v4i32 NEONimmAllZerosV))),
----------------
t2addrmode_imm7<0> -> t2addrmode_imm7<1>, for a VLDRH. Same below.


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:511
+      // Only support extending integers if the memory is aligned.
+      if ((EltWidth == 16 && Alignment < 2) ||
+          (EltWidth == 32 && Alignment < 4))
----------------
If this is coming from codegen, can the alignment here be 0? I think in ISel it is always set (and clang will always set it), but it may not be guaranteed in llvm in general.


================
Comment at: test/CodeGen/Thumb2/mve-masked-load.ll:551
+; CHECK-LE-NEXT:    vldrbt.s16 q0, [r0]
+; CHECK-LE-NEXT:    vpsel q0, q0, q1
+; CHECK-LE-NEXT:    bx lr
----------------
I don't think this vpsel should be here (it's not wrong, just inefficient, the instruction will already to this setting off predicated lanes to 0).

I'm guessing that the LowerMLOAD is creating a zero vector (that is potentially the wrong type?), so when it is called on the newly created maskedload it doesn't recognise it as 0 and we end up with the vselect being added too.


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

https://reviews.llvm.org/D68337