[X86] Add calculation for elements in structures in getting uniform base for the Gather/Scatter intrinsic.ClosedPublicActions

Authored by pengfei on Dec 12 2019, 4:28 PM.

Details

Summary

Add calculation for elements in structures in getting uniform base for the Gather/Scatter intrinsic.

Diff Detail

Repository
rG LLVM Github Monorepo
Build Status
 Buildable 42517 Build 43023: arc lint + arc unit

Event Timeline

pengfei created this revision.Dec 12 2019, 4:28 PM
Herald added a project: Restricted Project. Dec 12 2019, 4:28 PM
pengfei updated this revision to Diff 233730.Dec 12 2019, 7:08 PM

Updated to only check the final type.

pengfei marked an inline comment as done.Dec 12 2019, 7:32 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4360

I think for X86, we can leverage the displacement to handle indices aren't 0.

craig.topper added inline comments.Dec 12 2019, 7:32 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4371

The normal way to do this is to use gep_type_iterator in the loop as well and increment it as you go through the loop.

pengfei marked an inline comment as done.Dec 12 2019, 7:41 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4360

Oh, displacement is already used for base. Sorry for the noise.

pengfei updated this revision to Diff 233734.Dec 12 2019, 9:46 PM

Using gep_type_iterator to traverse.

pengfei marked an inline comment as done.Dec 12 2019, 9:47 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4371

Great, thanks!

craig.topper added inline comments.Dec 12 2019, 10:13 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4375

What is IndexVal isn't a constant?

4375

What if IndexVal isn't a constant?

pengfei updated this revision to Diff 233739.Dec 12 2019, 11:26 PM

We don't need to check if constant is defined.

pengfei marked an inline comment as done.Dec 12 2019, 11:27 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4375

Makes sence.

LuoYuanke added inline comments.Dec 13 2019, 12:01 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4375

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;```
craig.topper added inline comments.Dec 13 2019, 12:01 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4376

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.

pengfei marked 2 inline comments as done.Dec 13 2019, 12:46 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4375

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?

4376

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?

LuoYuanke added inline comments.Dec 13 2019, 3:09 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4375

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.

pengfei updated this revision to Diff 233925.Dec 14 2019, 1:40 AM

Optimized as Yuanke's suggestion.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4389

just 'cast'?

CI must be nonnull anyway, otherwise your assert fires.

xbolva00 added inline comments.Dec 14 2019, 2:37 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4385

Same here

pengfei marked an inline comment as done.Dec 14 2019, 3:04 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4389

I wonder if there's a real case that index of a structure is not a constant? I guess not, but I'm not so sure. So I put an assert followed.

pengfei marked an inline comment as done.Dec 14 2019, 3:21 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4389

From LangRef "When indexing into a (optionally packed) structure, only i32 integer constants are allowed (when using a vector of indices they must all be the same i32 integer constant)."
I think I can remove the assert.

pengfei updated this revision to Diff 233927.Dec 14 2019, 4:25 AM

Removed unnecessary assertions. Merged some common code.

pengfei updated this revision to Diff 233928.Dec 14 2019, 4:35 AM

Removed unnecessary else.

pengfei updated this revision to Diff 233931.Dec 14 2019, 5:53 AM

Removed unnecessary loop condition.
Added a vectorized index test.
Don't check if constant exist in DAG.

pengfei retitled this revision from [X86] Check if source elements are not structures before using a uniform base for the Gather/Scatter intrinsic. to [X86] Add calculation for elements in structures in getting uniform base for the Gather/Scatter intrinsic..Dec 14 2019, 7:12 AM
pengfei edited the summary of this revision. (Show Details)
craig.topper added inline comments.Dec 14 2019, 1:38 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4366

Can we just handle the FinalIndex cases after the loop instead of trying to special case the last iteration of the loop? It might require a tiny bit of duplication, but I think it would be more understandable.

pengfei marked an inline comment as done.Dec 14 2019, 5:28 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4366

Sure!

pengfei updated this revision to Diff 233947.Dec 14 2019, 5:31 PM

Ping.

LGTM

Hi Craig,

craig.topper added inline comments.Dec 17 2019, 6:41 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4376

use isa instead of dyn_cast if you don't need the casted pointer.

4376

Isn't GEP->getOperand(FinalIndex) already in IndexVal?

4386

Can this just be cast<Constant>(IndexVal)?

pengfei updated this revision to Diff 234447.Dec 17 2019, 7:58 PM