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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 6 inline comments as done.
c-rhodes added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:5524-5531
+  SDValue Chain = Expanded->getChain();
+  SDValue Ptr = Expanded->getOperand(1);
+
+  // Load the lo part of the spliced result
+  Lo = DAG.getLoad(LoVT, DL, Chain, Ptr, PtrInfo);
+  IncrementPointer(cast<MemSDNode>(Lo), LoVT, PtrInfo, Ptr);
+  // Load the hi part of the spliced result
----------------
sdesmalen wrote:
> is there a way to split the load using existing code?
> is there a way to split the load using existing code?

Splitting the full load from `expandVectorSplice` with `SplitVecRes_LOAD` seems to work.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10911
+
+  uint64_t Idx = (NumElts + (Imm % NumElts)) % NumElts;
+
----------------
sdesmalen wrote:
> just `Imm % NumElts`; ?
> just `Imm % NumElts`; ?

I tried that at first but found the behaviour isn't correct for negative immediates, for example a trailing element count of `-15` and 16 elements `-15 % 16 = -15`, so end up with this mask: `[-15,-14,-13,-12,-11,-10,-9,-8,-7,-6,-5,-4,-3,-2,-1,0]`. From what I read the sign of the modulo result is implementation defined if one or both of the operands are are negative.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8657
+                              VT.getStoreSize().getKnownMinSize()));
+      TrailingBytes = DAG.getNode(ISD::UMIN, DL, PtrVT, TrailingBytes, VLBytes);
+    }
----------------
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`


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

https://reviews.llvm.org/D94708