[PATCH] D94708: [IR] Introduce llvm.experimental.vector.splice intrinsic

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Just a few more nits.



================
Comment at: llvm/docs/LangRef.rst:16307
+the source/result vector. The ``imm`` is a signed integer constant in the range
+``-VL <= imm < VL``, for values outside of this range the result is poison.
+
----------------
nit: `s/, for/. For/`


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:565
+  ///   RESULT[i] = CONCAT_VECTORS(VEC1,VEC2)[VEC1.ElementCount - ABS(IMM) + i]
+  /// If T is a fixed-width vector and IMM is out-of-bounds the result vector is
+  /// undefined. If T is a scalable vector, IMM is clamped by the runtime
----------------
nit: `s/T is [..] and//`


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:565
+  ///   RESULT[i] = CONCAT_VECTORS(VEC1,VEC2)[VEC1.ElementCount - ABS(IMM) + i]
+  /// If T is a fixed-width vector and IMM is out-of-bounds the result vector is
+  /// undefined. If T is a scalable vector, IMM is clamped by the runtime
----------------
sdesmalen wrote:
> nit: `s/T is [..] and//`
nit: `s/out-of-bounds/not within the range [-VL, VL)/`


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:566-567
+  /// If T is a fixed-width vector and IMM is out-of-bounds the result vector is
+  /// undefined. If T is a scalable vector, IMM is clamped by the runtime
+  /// scaling factor 'vscale'. IMM is a constant integer.
+  VECTOR_SPLICE,
----------------
Please remove this restriction for scalable vectors. The implementation (by clamping to avoid a runtime crash) should not dictate the specification. It is sufficient to say that if IMM is out of range that the result value is undefined.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8644
+
+  if (Imm < 0) {
+    uint64_t TrailingElts = -Imm;
----------------
nit: because the Imm < 0 block is a lot bigger, maybe you can rewrite it as:
  if (Imm >= 0) {
    // Load back the required element.
    StackPtr = getVectorElementPointer(DAG, StackPtr, VT, Node->getOperand(2));
    // Load the spliced result
    return DAG.getLoad(VT, DL, StoreV2, StackPtr,
                       MachinePointerInfo::getUnknownStack(MF));
  }
  // Handle Imm < 0 case here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8657
+                              VT.getStoreSize().getKnownMinSize()));
+      TrailingBytes = DAG.getNode(ISD::UMIN, DL, PtrVT, TrailingBytes, VLBytes);
+    }
----------------
c-rhodes wrote:
> sdesmalen wrote:
> > You'll need to do a similar clamping for Imm > 0 (or actually, Imm > MinKnownVL) , to avoid loading beyond the allocated stack object.
> > You'll need to do a similar clamping for Imm > 0 (or actually, Imm > MinKnownVL) , to avoid loading beyond the allocated stack object.
> 
> For the index `getVectorElementPointer` does the clamping via `clampDynamicVectorIndex`
Thanks for confirming. Can you just add a comment that clarifies this?


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

https://reviews.llvm.org/D94708