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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei marked 2 inline comments as done.
pengfei added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4375
+  if (GTI.isStruct())
+    return false;
+
----------------
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?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4381
+    return false;
+  if (!dyn_cast<Constant>(GEP->getOperand(FinalIndex)) &&
+      !SDB->findValue(IndexVal))
----------------
craig.topper wrote:
> Does the test cover this change? I was just pointing out a mistake in the FIXME. I wasn't trying to ask for a code change unless its relevant to fixing the bug the test is for.
Not currently. But there's a bit relationship. Without this change, I need to *make* the same constant somewhere to get the excepted result.
```
  %4 = icmp eq <8 x i32> %trigger, <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
```
I think I can remove the constant to cover this change. I can also keep the FIXME. 
Which one do you think it's better?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71442