Page MenuHomePhabricator

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

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.

Event Timeline

pengfei created this revision.Dec 12 2019, 4:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4358

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
4369

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4358

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4369

Great, thanks!

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

What is IndexVal isn't a constant?

4373

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4373

Makes sence.

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

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
4374

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4373

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?

4374

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
4373

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.

xbolva00 added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4387

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
4383

Same here

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

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4387

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
4364

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
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4364

Sure!

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

Address review comments.

Ping.

LGTM

Hi Craig,
How about you?

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

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

4374

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

4384

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

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

Address review comments.

pengfei marked 3 inline comments as done.Dec 17 2019, 7:58 PM
craig.topper accepted this revision.Dec 17 2019, 8:03 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Dec 17 2019, 8:03 PM
This revision was automatically updated to reflect the committed changes.

I should have a fix for this in a few minutes.