[PATCH] D108115: [DAG][sve] Lowering for VLS masked truncating stores

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Sorry for the late review @DavidTruby.  Although I believe the patch as is likely works fine I've spotted an inconsistency that if possible would good to fix.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10109
+    return DAG.getMaskedStore(Chain, SDLoc(N), Value.getOperand(0), Ptr,
+                              MST->getOffset(), MST->getMask(),
+                              MST->getMemoryVT(), MST->getMemOperand(),
----------------
I have a comment below where I'm suggesting passing the original mask unmodified is likely an error.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18870-18876
 
+  if (Store->isTruncatingStore()) {
+    Mask = DAG.getNode(
+        ISD::SIGN_EXTEND, DL,
+        VT.changeVectorElementType(ContainerVT.getVectorElementType()), Mask);
+  }
+  Mask = convertFixedMaskToScalableVector(Mask, DAG);
----------------
On reflection I think this might be a bug introduced by the change to `DAGCombiner::visitMSTORE`. I say "might" because the requirements for `MSTORE`'s mask is not well documented.

Typically a mask's type is linked to the main datatype of the operation.  In this instance the MSTORE's main datatype is the value being stored and thus I believe the mask's VT should always be the result of `getSetCCResultType(Value->getValueType())`.

This is something you can see the legaliser honouring, where it uses the helper function `DAGTypeLegalizer::PromoteTargetBoolean`.  I think visitMSTORE should be using similar logic when folding in a truncate. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108115