[SelectionDAGBuilder] Don't set MachinePointerInfo for gather when we find a uniform base

[craig.topper]

Summary

I believe we were previously calculating a pointer info with the scalar base and an offset of 0. But that's not really where the gather is pointing. The offset is a function of the indices of the GEP we looked through.

Also get rid of the constant memory stuff for gather because I'm not sure that was right either.

I'm also not sure the memory size for the gather/scatter is correct either since I think it using the vector VT, but we're really loading/storing multiple disjoint scalars.

[efriedma]

We want to try to create the best MachineMemOperand we can. That means:

1. In general, we can't express the semantics in MachinePointerInfo; the Value has to be a pointer, not a vector of pointers. So we have to create an "empty" MachinePointerInfo. (But we should make sure to note the address-space correctly.)
2. If we find a uniform base, we should be able to express the notion that each loaded value has to be "based on" that pointer. I'm not sure we can just pass it to the MachinePointerInfo; probably we need some way to express that the offsets could be negative. But it shouldn't be a big extension. Even if the access offset/size is unknown, we can still figure out some useful aliasing information. (Two pointers to distinct objects can't alias.)
3. The size of the access should reflect the actual distance between the accesses, not the number of bytes accessed. If we don't have any way to express an access of unknown size, we should add it (it would be useful in other contexts). Maybe -1ULL just works?

---

I'm not sure what you think is wrong with the pointsToConstantMemory check. If a gather loads from constant memory, it obviously can't alias anything.

---

We should be able to write testcases for all of this, I think; there are DAGCombines that depend on whether we can prove aliasing between a load and a store.

[craig.topper]

In D76157#1922361, @efriedma wrote:

We want to try to create the best MachineMemOperand we can. That means:

1. In general, we can't express the semantics in MachinePointerInfo; the Value has to be a pointer, not a vector of pointers. So we have to create an "empty" MachinePointerInfo. (But we should make sure to note the address-space correctly.)

Ok I'll fix the address space.

2. If we find a uniform base, we should be able to express the notion that each loaded value has to be "based on" that pointer. I'm not sure we can just pass it to the MachinePointerInfo; probably we need some way to express that the offsets could be negative. But it shouldn't be a big extension. Even if the access offset/size is unknown, we can still figure out some useful aliasing information. (Two pointers to distinct objects can't alias.)
3. The size of the access should reflect the actual distance between the accesses, not the number of bytes accessed. If we don't have any way to express an access of unknown size, we should add it (it would be useful in other contexts). Maybe -1ULL just works?

I suspect the type legalization code for splitting gathers/scatters is also miscalculating the size. It just calls getStoreSize() on the split VT pieces.

---

I'm not sure what you think is wrong with the pointsToConstantMemory check. If a gather loads from constant memory, it obviously can't alias anything.

I wasn't sure if passing just the "base" pointer was correct.

---

We should be able to write testcases for all of this, I think; there are DAGCombines that depend on whether we can prove aliasing between a load and a store.

Do any of those DAG combines even consider gather/scatter nodes or do they just look at loadsdnode/storesdnode?

[efriedma]

Do any of those DAG combines even consider gather/scatter nodes or do they just look at loadsdnode/storesdnode?

Oh, hmm, maybe we don't actually try to use aliasing to improve chains past a gather/scatter at the moment. I thought we just had some generic code for MemSDNodes, but I looked and I'm not seeing it.

[efriedma]

I wasn't sure if passing just the "base" pointer was correct.

It doesn't really matter what the MachineMemOperand looks like for an operation that doesn't alias anything. :)

[craig.topper]

In D76157#1922467, @efriedma wrote:

I wasn't sure if passing just the "base" pointer was correct.

It doesn't really matter what the MachineMemOperand looks like for an operation that doesn't alias anything. :)

Sorry not the MachineMemOperand, the MemoryLocation we use to call AA->pointsToConstantMemory is using the uniform base ignoring the GEP indices. Is that safe?

[efriedma]

I don't think pointsToConstantMemory actually pays attention to the offset/size at the moment, so it's probably safe in practice. Theoretically, I guess we're passing the wrong size/offset, yes.

[craig.topper]

Preserve address space and change size to UnknownSize.

[efriedma]

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 //  or
 //   %gep.ptr = getelementptr i32, i32* %ptr,        <8 x i32> %ind
 // %res = call <8 x i32> @llvm.masked.gather.v8i32(<8 x i32*> %gep.ptr, ..
 //
 // When the first GEP operand is a single pointer - it is the uniform base we
 // are looking for. If first operand of the GEP is a splat vector - we
 // extract the splat value and use it as a uniform base.
 // In all other cases the function returns 'false'.
-static bool getUniformBase(const Value *&Ptr, SDValue &Base, SDValue &Index,
+static bool getUniformBase(const Value *Ptr, SDValue &Base, SDValue &Index,
                            ISD::MemIndexType &IndexType, SDValue &Scale,
                            SelectionDAGBuilder *SDB) {
   SelectionDAG& DAG = SDB->DAG;
   LLVMContext &Context = *DAG.getContext();
 
   assert(Ptr->getType()->isVectorTy() && "Uexpected pointer type");
   const GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr);
   if (!GEP)
     return false;
 
-  const Value *GEPPtr = GEP->getPointerOperand();
-  if (!GEPPtr->getType()->isVectorTy())
-    Ptr = GEPPtr;
-  else if (!(Ptr = getSplatValue(GEPPtr)))
+  const Value *BasePtr = GEP->getPointerOperand();
+  if (BasePtr->getType()->isVectorTy()) {
+    BasePtr = getSplatValue(BasePtr);
+    if (!BasePtr)
       return false;
+  }
 
   unsigned FinalIndex = GEP->getNumOperands() - 1;
   Value *IndexVal = GEP->getOperand(FinalIndex);
   gep_type_iterator GTI = gep_type_begin(*GEP);
 
   // Ensure all the other indices are 0.
   for (unsigned i = 1; i < FinalIndex; ++i, ++GTI) {
     auto *C = dyn_cast<Constant>(GEP->getOperand(i));
     if (!C)
       return false;
     if (isa<VectorType>(C->getType()))
       C = C->getSplatValue();
     auto *CI = dyn_cast_or_null<ConstantInt>(C);
     if (!CI || !CI->isZero())
       return false;
   }
 
   // The operands of the GEP may be defined in another basic block.
   // In this case we'll not find nodes for the operands.
-  if (!SDB->findValue(Ptr))
+  if (!SDB->findValue(BasePtr))
     return false;
   Constant *C = dyn_cast<Constant>(IndexVal);
   if (!C && !SDB->findValue(IndexVal))
     return false;
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   const DataLayout &DL = DAG.getDataLayout();
   StructType *STy = GTI.getStructTypeOrNull();
 
   if (STy) {
     const StructLayout *SL = DL.getStructLayout(STy);
     unsigned Field = cast<Constant>(IndexVal)->getUniqueInteger().getZExtValue();
     Scale = DAG.getTargetConstant(1, SDB->getCurSDLoc(), TLI.getPointerTy(DL));
     Index = DAG.getConstant(SL->getElementOffset(Field),
                             SDB->getCurSDLoc(), TLI.getPointerTy(DL));
   } else {
     Scale = DAG.getTargetConstant(
                 DL.getTypeAllocSize(GEP->getResultElementType()),
                 SDB->getCurSDLoc(), TLI.getPointerTy(DL));
     Index = SDB->getValue(IndexVal);
   }
-  Base = SDB->getValue(Ptr);
+  Base = SDB->getValue(BasePtr);
   IndexType = ISD::SIGNED_SCALED;
 
   if (STy || !Index.getValueType().isVector()) {
     unsigned GEPWidth = GEP->getType()->getVectorNumElements();
     EVT VT = EVT::getVectorVT(Context, Index.getValueType(), GEPWidth);
     Index = DAG.getSplatBuildVector(VT, SDLoc(Index), Index);
   }
   return true;
void SelectionDAGBuilder::visitMaskedScatter(const CallInst &I) {
 
   AAMDNodes AAInfo;
   I.getAAMetadata(AAInfo);
 
   SDValue Base;
   SDValue Index;
   ISD::MemIndexType IndexType;
   SDValue Scale;
-  const Value *BasePtr = Ptr;
-  bool UniformBase = getUniformBase(BasePtr, Base, Index, IndexType, Scale,
-                                    this);
+  bool UniformBase = getUniformBase(Ptr, Base, Index, IndexType, Scale, this);
 
-  const Value *MemOpBasePtr = UniformBase ? BasePtr : nullptr;
+  unsigned AS = Ptr->getType()->getScalarType()->getPointerAddressSpace();
   MachineMemOperand *MMO = DAG.getMachineFunction().
-    getMachineMemOperand(MachinePointerInfo(MemOpBasePtr),
+    getMachineMemOperand(MachinePointerInfo(AS),
                          MachineMemOperand::MOStore,
                          // TODO: Make MachineMemOperands aware of scalable
                          // vectors.
-                         VT.getStoreSize().getKnownMinSize(),
+                         MemoryLocation::UnknownSize,
                          Alignment, AAInfo);
   if (!UniformBase) {
     Base = DAG.getConstant(0, sdl, TLI.getPointerTy(DAG.getDataLayout()));
     Index = getValue(Ptr);
     IndexType = ISD::SIGNED_SCALED;
     Scale = DAG.getTargetConstant(1, sdl, TLI.getPointerTy(DAG.getDataLayout()));
   }
   SDValue Ops[] = { getMemoryRoot(), Src0, Mask, Base, Index, Scale };
void SelectionDAGBuilder::visitMaskedGather(const CallInst &I) {
   I.getAAMetadata(AAInfo);
   const MDNode *Ranges = I.getMetadata(LLVMContext::MD_range);
 
   SDValue Root = DAG.getRoot();
   SDValue Base;
   SDValue Index;
   ISD::MemIndexType IndexType;
   SDValue Scale;
-  const Value *BasePtr = Ptr;
-  bool UniformBase = getUniformBase(BasePtr, Base, Index, IndexType, Scale,
-                                    this);
-  bool ConstantMemory = false;
-  if (UniformBase && AA &&
-      AA->pointsToConstantMemory(
-          MemoryLocation(BasePtr,
-                         LocationSize::precise(
-                             DAG.getDataLayout().getTypeStoreSize(I.getType())),
-                         AAInfo))) {
-    // Do not serialize (non-volatile) loads of constant memory with anything.
-    Root = DAG.getEntryNode();
-    ConstantMemory = true;
-  }
-
+  bool UniformBase = getUniformBase(Ptr, Base, Index, IndexType, Scale, this);
+  unsigned AS = Ptr->getType()->getScalarType()->getPointerAddressSpace();
   MachineMemOperand *MMO =
     DAG.getMachineFunction().
-    getMachineMemOperand(MachinePointerInfo(UniformBase ? BasePtr : nullptr),
+    getMachineMemOperand(MachinePointerInfo(AS),
                          MachineMemOperand::MOLoad,
                          // TODO: Make MachineMemOperands aware of scalable
                          // vectors.
-                         VT.getStoreSize().getKnownMinSize(),
+                         MemoryLocation::UnknownSize,
                          Alignment, AAInfo, Ranges);
 
   if (!UniformBase) {
     Base = DAG.getConstant(0, sdl, TLI.getPointerTy(DAG.getDataLayout()));
     Index = getValue(Ptr);
     IndexType = ISD::SIGNED_SCALED;
     Scale = DAG.getTargetConstant(1, sdl, TLI.getPointerTy(DAG.getDataLayout()));
   }
   SDValue Ops[] = { Root, Src0, Mask, Base, Index, Scale };
   SDValue Gather = DAG.getMaskedGather(DAG.getVTList(VT, MVT::Other), VT, sdl,
                                        Ops, MMO, IndexType);
 
-  SDValue OutChain = Gather.getValue(1);
-  if (!ConstantMemory)
-    PendingLoads.push_back(OutChain);
+  PendingLoads.push_back(Gather.getValue(1));
   setValue(&I, Gather);
 }
 
 void SelectionDAGBuilder::visitAtomicCmpXchg(const AtomicCmpXchgInst &I) {
   SDLoc dl = getCurSDLoc();
   AtomicOrdering SuccessOrdering = I.getSuccessOrdering();
   AtomicOrdering FailureOrdering = I.getFailureOrdering();
   SyncScope::ID SSID = I.getSyncScopeID();