-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[LV] Add initial support for vectorizing literal struct return values #109833
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 all commits
6e45274
178b2c3
c0f9813
cc7e82a
6f1e15c
4b1534e
e5af9fc
2ee07ab
af8febd
8ddfd70
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 |
---|---|---|
|
@@ -336,10 +336,10 @@ Value *VPTransformState::get(VPValue *Def, bool NeedsScalar) { | |
} else { | ||
// Initialize packing with insertelements to start from undef. | ||
assert(!VF.isScalable() && "VF is assumed to be non scalable."); | ||
Value *Undef = PoisonValue::get(VectorType::get(LastInst->getType(), VF)); | ||
Value *Undef = PoisonValue::get(toVectorizedTy(LastInst->getType(), VF)); | ||
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. If you want, you could potentially pull out some of these changes into a NFC patch. For example, toVectorTy -> toVectorizedTy and VectorType::get -> toVectorizedTy, etc. Although if you don't think it's worth it I'm happy as it is. 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 spilling those out would make sense. Within this patch, all of these changes are needed for the core functionality to work (switching the call does change behavior). If I landed these as an NFC, it makes it less obvious that these are required changes for the struct vectorization. |
||
set(Def, Undef); | ||
for (unsigned Lane = 0; Lane < VF.getKnownMinValue(); ++Lane) | ||
packScalarIntoVectorValue(Def, Lane); | ||
packScalarIntoVectorizedValue(Def, Lane); | ||
VectorValue = get(Def); | ||
} | ||
Builder.restoreIP(OldIP); | ||
|
@@ -392,13 +392,24 @@ void VPTransformState::setDebugLocFrom(DebugLoc DL) { | |
Builder.SetCurrentDebugLocation(DIL); | ||
} | ||
|
||
void VPTransformState::packScalarIntoVectorValue(VPValue *Def, | ||
const VPLane &Lane) { | ||
void VPTransformState::packScalarIntoVectorizedValue(VPValue *Def, | ||
const VPLane &Lane) { | ||
Value *ScalarInst = get(Def, Lane); | ||
Value *VectorValue = get(Def); | ||
VectorValue = Builder.CreateInsertElement(VectorValue, ScalarInst, | ||
Lane.getAsRuntimeExpr(Builder, VF)); | ||
set(Def, VectorValue); | ||
Value *WideValue = get(Def); | ||
Value *LaneExpr = Lane.getAsRuntimeExpr(Builder, VF); | ||
if (auto *StructTy = dyn_cast<StructType>(WideValue->getType())) { | ||
// We must handle each element of a vectorized struct type. | ||
for (unsigned I = 0, E = StructTy->getNumElements(); I != E; I++) { | ||
Value *ScalarValue = Builder.CreateExtractValue(ScalarInst, I); | ||
Value *VectorValue = Builder.CreateExtractValue(WideValue, I); | ||
VectorValue = | ||
Builder.CreateInsertElement(VectorValue, ScalarValue, LaneExpr); | ||
WideValue = Builder.CreateInsertValue(WideValue, VectorValue, I); | ||
} | ||
} else { | ||
WideValue = Builder.CreateInsertElement(WideValue, ScalarInst, LaneExpr); | ||
} | ||
set(Def, WideValue); | ||
} | ||
|
||
BasicBlock *VPBasicBlock::createEmptyBasicBlock(VPTransformState &State) { | ||
|
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.
I think it would be good to have some cost model tests showing what happens in different scenarios when you have call instructions in a loop that return a struct. In particular, it would be good to have tests that exercise this code path.
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.
Just checking if this has been addressed in the latest version?
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.
Yes, there's some cost model tests and a test case crafted to hit this specific code path too (
@scalarized_predicated_struct_return
).