-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[X86][SelectionDAG] Fix the Gather's base and index by modifying the Scale value #137813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 31 commits
741acb0
9d395b1
0625ae4
84d9a5f
76d9167
b73051c
a43aa1d
03b3daf
0e8856f
7eb7663
4ed4a4d
0c4eb0f
dc1b6d6
34df281
5aaa2ab
8ce9360
bfbbe9f
4bb9f5b
fb131f8
47bf70b
716943e
b9e571a
f0a175a
4b57e4a
8995df9
5a12c7d
d2f1352
ed72aa3
1644def
7ca7d48
94f16e6
cfcf542
7ab9924
62fa5ad
ad2e419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56652,31 +56652,81 @@ static SDValue combineGatherScatter(SDNode *N, SelectionDAG &DAG, | |
const TargetLowering &TLI = DAG.getTargetLoweringInfo(); | ||
|
||
if (DCI.isBeforeLegalize()) { | ||
// Attempt to move shifted index into the address scale, allows further | ||
// index truncation below. | ||
if (Index.getOpcode() == ISD::SHL && isa<ConstantSDNode>(Scale)) { | ||
unsigned BitWidth = Index.getScalarValueSizeInBits(); | ||
unsigned ScaleAmt = Scale->getAsZExtVal(); | ||
assert(isPowerOf2_32(ScaleAmt) && "Scale must be a power of 2"); | ||
unsigned Log2ScaleAmt = Log2_32(ScaleAmt); | ||
RKSimon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
unsigned MaskBits = BitWidth - Log2ScaleAmt; | ||
APInt DemandedBits = APInt::getLowBitsSet(BitWidth, MaskBits); | ||
if (TLI.SimplifyDemandedBits(Index, DemandedBits, DCI)) { | ||
if (N->getOpcode() != ISD::DELETED_NODE) | ||
DCI.AddToWorklist(N); | ||
return SDValue(N, 0); | ||
} | ||
if (auto MinShAmt = DAG.getValidMinimumShiftAmount(Index)) { | ||
if (*MinShAmt >= 1 && (*MinShAmt + Log2ScaleAmt) < 4 && | ||
DAG.ComputeNumSignBits(Index.getOperand(0)) > 1) { | ||
SDValue ShAmt = Index.getOperand(1); | ||
SDValue NewShAmt = | ||
DAG.getNode(ISD::SUB, DL, ShAmt.getValueType(), ShAmt, | ||
DAG.getConstant(1, DL, ShAmt.getValueType())); | ||
SDValue NewIndex = DAG.getNode(ISD::SHL, DL, Index.getValueType(), | ||
Index.getOperand(0), NewShAmt); | ||
SDValue NewScale = | ||
DAG.getConstant(ScaleAmt * 2, DL, Scale.getValueType()); | ||
return rebuildGatherScatter(GorS, NewIndex, Base, NewScale, DAG); | ||
} | ||
} | ||
} | ||
unsigned IndexWidth = Index.getScalarValueSizeInBits(); | ||
|
||
// Shrink indices if they are larger than 32-bits. | ||
// Only do this before legalize types since v2i64 could become v2i32. | ||
// FIXME: We could check that the type is legal if we're after legalize | ||
// types, but then we would need to construct test cases where that happens. | ||
if (IndexWidth > 32 && DAG.ComputeNumSignBits(Index) > (IndexWidth - 32)) { | ||
EVT NewVT = IndexVT.changeVectorElementType(MVT::i32); | ||
|
||
// FIXME: We could support more than just constant fold, but we need to | ||
// careful with costing. A truncate that can be optimized out would be | ||
// fine. Otherwise we might only want to create a truncate if it avoids a | ||
// split. | ||
if (SDValue TruncIndex = | ||
DAG.FoldConstantArithmetic(ISD::TRUNCATE, DL, NewVT, Index)) | ||
return rebuildGatherScatter(GorS, TruncIndex, Base, Scale, DAG); | ||
|
||
// Shrink any sign/zero extends from 32 or smaller to larger than 32 if | ||
// there are sufficient sign bits. Only do this before legalize types to | ||
// avoid creating illegal types in truncate. | ||
if ((Index.getOpcode() == ISD::SIGN_EXTEND || | ||
Index.getOpcode() == ISD::ZERO_EXTEND) && | ||
Index.getOperand(0).getScalarValueSizeInBits() <= 32) { | ||
Index = DAG.getNode(ISD::TRUNCATE, DL, NewVT, Index); | ||
return rebuildGatherScatter(GorS, Index, Base, Scale, DAG); | ||
// \ComputeNumSignBits value is recomputed for the shift Index | ||
if (IndexWidth > 32) { | ||
// If the index is a left shift, \ComputeNumSignBits we are recomputing | ||
// the number of sign bits from the shifted value. We are trying to enable | ||
// the optimization in which we can shrink indices if they are larger than | ||
// 32-bits. Using the existing fold techniques implemented below. | ||
unsigned ComputeNumSignBits = DAG.ComputeNumSignBits(Index); | ||
if (Index.getOpcode() == ISD::SHL) { | ||
if (auto MinShAmt = DAG.getValidMinimumShiftAmount(Index)) { | ||
if (DAG.ComputeNumSignBits(Index.getOperand(0)) > 1) { | ||
ComputeNumSignBits += *MinShAmt; | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RKSimon, I do not understand the comment fully... Does it mean to move it outside IndexWith > 32 or Does it mean to remove this code snippet? We can not remove this code as it is required and our test case is failing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In which case, please can you pull it out of this patch - I'm happy with the rest of it, but I'm concerned that this will only work under very specific conditions - better to pull it out and you can work on it in a follow up PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incase of SHL have the value of shift 4 or greater... We are recalculating the number of signed bit so that we can truncate. Sure, i will create a separate PR for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, removed the code and created a separate PR. |
||
if (ComputeNumSignBits > (IndexWidth - 32)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably better if you can move everything inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RKSimon, Done. Moved the code inside the if (IndexWidth > 32) |
||
EVT NewVT = IndexVT.changeVectorElementType(MVT::i32); | ||
|
||
// FIXME: We could support more than just constant fold, but we need to | ||
// careful with costing. A truncate that can be optimized out would be | ||
// fine. Otherwise we might only want to create a truncate if it avoids | ||
// a split. | ||
if (SDValue TruncIndex = | ||
DAG.FoldConstantArithmetic(ISD::TRUNCATE, DL, NewVT, Index)) | ||
return rebuildGatherScatter(GorS, TruncIndex, Base, Scale, DAG); | ||
|
||
// Shrink any sign/zero extends from 32 or smaller to larger than 32 if | ||
// there are sufficient sign bits. Only do this before legalize types to | ||
// avoid creating illegal types in truncate. | ||
if ((Index.getOpcode() == ISD::SIGN_EXTEND || | ||
Index.getOpcode() == ISD::ZERO_EXTEND) && | ||
Index.getOperand(0).getScalarValueSizeInBits() <= 32) { | ||
Index = DAG.getNode(ISD::TRUNCATE, DL, NewVT, Index); | ||
return rebuildGatherScatter(GorS, Index, Base, Scale, DAG); | ||
} | ||
|
||
// Shrink if we remove an illegal type. | ||
if (!TLI.isTypeLegal(Index.getValueType()) && TLI.isTypeLegal(NewVT)) { | ||
Index = DAG.getNode(ISD::TRUNCATE, DL, NewVT, Index); | ||
return rebuildGatherScatter(GorS, Index, Base, Scale, DAG); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoist the equivalent IndexWidth above this and remove BitWidth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done