[VP] Propagate align parameter attr on VP gather/scatter to ISel

[frasercrmck]

Summary

This patch fixes a case where the 'align' parameter attribute on the
pointer operands to llvm.vp.gather and llvm.vp.scatter was being dropped
during the conversion to the SelectionDAG. The default alignment equal
to the ABI type alignment of the vector type was kept. It also updates
the documentation to reflect the fact that the parameter attribute is
now properly supported.

The default alignment of these intrinsics was previously documented as
being equal to the ABI alignment of the *scalar* type, when in fact that
wasn't the case: the ABI alignment of the vector type was used instead.
This has also been fixed in this patch.

[craig.topper]

LGTM

[frasercrmck]

• rebase and use 'operand' over 'argument'

[simoll]

LGTM

[simoll]

What's our status with this patch?
I know we have been discussing among VP folks to get this approved by somebody who had worked on align before.

[frasercrmck]

In D114423#3173046, @simoll wrote:

What's our status with this patch?
I know we have been discussing among VP folks to get this approved by somebody who had worked on align before.

It's in a stack on top of another patch that hasn't yet been approved. I'm also open to finding someone more closely related to align and/or IR semantics but haven't done the work to find out who that might be.

[frasercrmck]

Aha I just found D87304 - maybe @jdoerfert @fhahn @anna @dantrushin would be interested in this patch.

[anna]

In D114423#3173134, @frasercrmck wrote:

Aha I just found D87304 - maybe @jdoerfert @fhahn @anna @dantrushin would be interested in this patch.

Hi Fraser,
Yes, this definitely unblocks some downstream only changes we had (basically partially reverting D87304) since we wanted to apply align to vector of pointers. I believe @jdoerfert wanted to see this applied to (at least one) other attribute apart from align.
The LangRef update looks good and clear to me.

I think having this as a first step is good unblocks our use case downstream as well as yours. There were some IR compatibility changes landed in follow-up patch, attached to D87304. Are those relevant here?

[anna]

Actually looking at the patch again, you're mixing two concepts here:

1. fixing the LangRef and Attributes.cpp for align attribute on vector of pointers should be done as an independent patch. It is orthogonal to the SelectionDAGBuilder code and avoids the false dependency created by the parent patch linked to this patch.
2. ABI intrinsics and SelectionDAG builder changes can depend on the parent patch.

For #1, You need couple of tests for the attributes change and at least one in Verifier.

[frasercrmck]

• rebase on D115161

[arsenm]

Should also update IRTranslator

[frasercrmck]

In D114423#3173815, @arsenm wrote:

Should also update IRTranslator

Sorry, I'm not best versed in GlobalISel. Just to check, do you mean for VP intrinsics? Or for the align parameter being applied to vectors of pointers (D115161)?

[frasercrmck]

• rebase, fix conflicts

[frasercrmck]

• rebase

@anna are you happy to unblock this review?

[frasercrmck]

ping @anna, thanks

[anna]

Looks like the comments have been addressed regarding the split. Thank you! Moving myself out of blocking state.

llvm/docs/LangRef.rst

 """"""""""
 
 The first operand is a vector of pointers which holds all memory addresses to
 read. The second operand is a vector of boolean values with the same number of
 elements as the return type. The third is the explicit vector length of the
 operation. The return type and underlying type of the vector of pointers are
 the same vector types.
 
+The :ref:`align <attr_align>` parameter attribute can be provided for the first
+operand.
+
 Semantics:
 """"""""""
 
 The '``llvm.vp.gather``' intrinsic reads multiple scalar values from memory in
 the same way as the '``llvm.masked.gather``' intrinsic, where the mask is taken
 from the combination of the '``mask``' and '``evl``' operands in the usual VP
-way. Of the '``llvm.masked.gather``' operands not set by '``llvm.vp.gather``':
-the '``passthru``' operand is implicitly ``undef``; the '``alignment``' operand
-is taken as the ABI alignment of the source addresses as specified by the
-:ref:`datalayout string<langref_datalayout>`.
+way. Certain '``llvm.masked.gather``' operands do not have corresponding
+operands in '``llvm.vp.gather``': the '``passthru``' operand is implicitly
+``undef``; the '``alignment``' operand is taken as the ``align`` parameter, if
+provided. The default alignment is taken as the ABI alignment of the source
+addresses as specified by the :ref:`datalayout string<langref_datalayout>`.
 
 Examples:
 """""""""
 
 .. code-block:: text
 
-     %r = call void @llvm.vp.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> %ptrs, <8 x i1> %mask, i32 %evl)
+     %r = call void @llvm.vp.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> align 8 %ptrs, <8 x i1> %mask, i32 %evl)
      ;; For all lanes below %evl, %r is lane-wise equivalent to %also.r
-     ;; Note that since the alignment is ultimately up to the data layout
-     ;; string, 8 is used as an example.
 
      %also.r = call void @llvm.masked.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> %ptrs, i32 8, <8 x i1> %mask, <8 x i8> undef)
 
 
 .. _int_vp_scatter:
 
 '``llvm.vp.scatter``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

 """"""""""
 
 The first operand is a vector value to be written to memory. The second operand
 is a vector of pointers, pointing to where the value elements should be stored.
 The third operand is a vector of boolean values with the same number of
 elements as the return type. The fourth is the explicit vector length of the
 operation.
 
+The :ref:`align <attr_align>` parameter attribute can be provided for the
+second operand.
+
 Semantics:
 """"""""""
 
 The '``llvm.vp.scatter``' intrinsic writes multiple scalar values to memory in
 the same way as the '``llvm.masked.scatter``' intrinsic, where the mask is
 taken from the combination of the '``mask``' and '``evl``' operands in the
-usual VP way. The '``alignment``' operand of the '``llvm.masked.scatter``'
-intrinsic is not set by '``llvm.vp.scatter``': it is taken as the ABI alignment
-of the destination addresses as specified by the :ref:`datalayout
+usual VP way. The '``alignment``' operand of the '``llvm.masked.scatter``' does
+not have a corresponding operand in '``llvm.vp.scatter``': it is instead
+provided via the optional ``align`` parameter attribute on the
+vector-of-pointers operand. Otherwise it is taken as the ABI alignment of the
+destination addresses as specified by the :ref:`datalayout
 string<langref_datalayout>`.
 
 Examples:
 """""""""
 
 .. code-block:: text
 
-     call void @llvm.vp.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> %ptrs, <8 x i1> %mask, i32 %evl)
+     call void @llvm.vp.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> align 1 %ptrs, <8 x i1> %mask, i32 %evl)
      ;; For all lanes below %evl, the call above is lane-wise equivalent to the call below.
-     ;; Note that since the alignment is ultimately up to the data layout
-     ;; string, 8 is used as an example.
 
-     call void @llvm.masked.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> %ptrs, i32 8, <8 x i1> %mask)
+     call void @llvm.masked.scatter.v8i8.v8p0i8(<8 x i8> %val, <8 x i8*> %ptrs, i32 1, <8 x i1> %mask)
 
 
 .. _int_mload_mstore:
 
 Masked Vector Load and Store Intrinsics
 ---------------------------------------
 
 LLVM provides intrinsics for predicated vector load and store operations. The predicate is specified by a mask operand, which holds one bit per vector element, switching the associated vector lane on or off. The memory addresses corresponding to the "off" lanes are not accessed. When all bits of the mask are on, the intrinsic is identical to a regular vector load or store. When all bits are off, no memory is accessed.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 
 void SelectionDAGBuilder::visitVPLoadGather(const VPIntrinsic &VPIntrin, EVT VT,
                                             SmallVector<SDValue, 7> &OpValues,
                                             bool IsGather) {
   SDLoc DL = getCurSDLoc();
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   Value *PtrOperand = VPIntrin.getArgOperand(0);
   MaybeAlign Alignment = VPIntrin.getPointerAlignment();
-  if (!Alignment)
-    Alignment = DAG.getEVTAlign(VT);
   AAMDNodes AAInfo = VPIntrin.getAAMetadata();
   const MDNode *Ranges = VPIntrin.getMetadata(LLVMContext::MD_range);
   SDValue LD;
   bool AddToChain = true;
   if (!IsGather) {
     // Do not serialize variable-length loads of constant memory with
     // anything.
+    if (!Alignment)
+      Alignment = DAG.getEVTAlign(VT);
     MemoryLocation ML;
     if (VT.isScalableVector())
       ML = MemoryLocation::getAfter(PtrOperand);
     else
       ML = MemoryLocation(
           PtrOperand,
           LocationSize::precise(
               DAG.getDataLayout().getTypeStoreSize(VPIntrin.getType())),
           AAInfo);
     AddToChain = !AA || !AA->pointsToConstantMemory(ML);
     SDValue InChain = AddToChain ? DAG.getRoot() : DAG.getEntryNode();
     MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
         MachinePointerInfo(PtrOperand), MachineMemOperand::MOLoad,
         VT.getStoreSize().getKnownMinSize(), *Alignment, AAInfo, Ranges);
     LD = DAG.getLoadVP(VT, DL, InChain, OpValues[0], OpValues[1], OpValues[2],
                        MMO, false /*IsExpanding */);
   } else {
+    if (!Alignment)
+      Alignment = DAG.getEVTAlign(VT.getScalarType());
     unsigned AS =
         PtrOperand->getType()->getScalarType()->getPointerAddressSpace();
     MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
         MachinePointerInfo(AS), MachineMemOperand::MOLoad,
         MemoryLocation::UnknownSize, *Alignment, AAInfo, Ranges);
     SDValue Base, Index, Scale;
     ISD::MemIndexType IndexType;
     bool UniformBase = getUniformBase(PtrOperand, Base, Index, IndexType, Scale,

 void SelectionDAGBuilder::visitVPStoreScatter(const VPIntrinsic &VPIntrin,
                                               SmallVector<SDValue, 7> &OpValues,
                                               bool IsScatter) {
   SDLoc DL = getCurSDLoc();
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   Value *PtrOperand = VPIntrin.getArgOperand(1);
   EVT VT = OpValues[0].getValueType();
   MaybeAlign Alignment = VPIntrin.getPointerAlignment();
-  if (!Alignment)
-    Alignment = DAG.getEVTAlign(VT);
   AAMDNodes AAInfo = VPIntrin.getAAMetadata();
   SDValue ST;
   if (!IsScatter) {
+    if (!Alignment)
+      Alignment = DAG.getEVTAlign(VT);
     MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
         MachinePointerInfo(PtrOperand), MachineMemOperand::MOStore,
         VT.getStoreSize().getKnownMinSize(), *Alignment, AAInfo);
     ST =
         DAG.getStoreVP(getMemoryRoot(), DL, OpValues[0], OpValues[1],
                        OpValues[2], OpValues[3], MMO, false /* IsTruncating */);
   } else {
+    if (!Alignment)
+      Alignment = DAG.getEVTAlign(VT.getScalarType());
     unsigned AS =
         PtrOperand->getType()->getScalarType()->getPointerAddressSpace();
     MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
         MachinePointerInfo(AS), MachineMemOperand::MOStore,
         MemoryLocation::UnknownSize, *Alignment, AAInfo);
     SDValue Base, Index, Scale;
     ISD::MemIndexType IndexType;
     bool UniformBase = getUniformBase(PtrOperand, Base, Index, IndexType, Scale,