[PATCH] D71442: [X86] Check if source elements are not structures before using a uniform base for the Gather/Scatter intrinsic.

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4375
+  if (GTI.isStruct())
+    return false;
+
----------------
pengfei wrote:
> LuoYuanke wrote:
> > Maybe we can calculate the offset for struct if the index is constant.
> > 
> >     APInt Offset(DL.getIndexTypeSizeInBits(GEP->getType()), 0);
> >     if (!GEP->accumulateConstantOffset(DL, Offset))
> >       return false;
> >     Index = DAG.getTargetConstant(Offset, SDB->getCurSDLoc(), TLI.getPointerTy(DL));
> >     Scale = DAG.getTargetConstant(1, SDB->getCurSDLoc(), TLI.getPointerTy(DL));
> > 
> > Also recognize zeroinitializer in accumulateConstantOffset().
> > 
> > --- a/llvm/lib/IR/Operator.cpp
> > +++ b/llvm/lib/IR/Operator.cpp
> > @@ -39,6 +39,9 @@ bool GEPOperator::accumulateConstantOffset(const DataLayout &DL,
> > 
> >    for (gep_type_iterator GTI = gep_type_begin(this), GTE = gep_type_end(this);
> >         GTI != GTE; ++GTI) {
> > +
> > +    if (isa<ConstantAggregateZero>(GTI.getOperand()))
> > +      continue;
> >      ConstantInt *OpC = dyn_cast<ConstantInt>(GTI.getOperand());
> >      if (!OpC)
> >        return false;
> > 
> Index inside a structure is always constant.
> I think your suggestions is reasonable. We can calculate the offset for a given constant index.
> Besides, the intermediate non zero constant index still can be calculated as your suggestions.
> But it looks like more optimization than bug fix. It's better to be implemented in another patch.
> 
> Why we need recognize zeroinitializer?
Yes. I agree it is better to be implemented in another patch.

<<<Why we need recognize zeroinitializer?
Because in GEPOperator::accumulateConstantOffset() it only support scalar index, not support splat vector index. Checking zeroinitializer is to take the index as scalar zero.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71442