-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[VPlan] Fold common edges away in convertPhisToBlends #150368
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
base: main
Are you sure you want to change the base?
Changes from all commits
4505704
1fd1624
be337df
7af8137
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 | ||||
---|---|---|---|---|---|---|
|
@@ -14,6 +14,7 @@ | |||||
#include "VPRecipeBuilder.h" | ||||||
#include "VPlan.h" | ||||||
#include "VPlanCFG.h" | ||||||
#include "VPlanPatternMatch.h" | ||||||
#include "VPlanTransforms.h" | ||||||
#include "VPlanUtils.h" | ||||||
#include "llvm/ADT/PostOrderIterator.h" | ||||||
|
@@ -65,6 +66,10 @@ class VPPredicator { | |||||
return EdgeMaskCache[{Src, Dst}] = Mask; | ||||||
} | ||||||
|
||||||
/// Given a widened phi \p PhiR, try to see if its incoming blocks all share a | ||||||
/// common edge and return its mask. | ||||||
VPValue *findCommonEdgeMask(const VPWidenPHIRecipe *PhiR) const; | ||||||
|
||||||
public: | ||||||
/// Returns the precomputed predicate of the edge from \p Src to \p Dst. | ||||||
VPValue *getEdgeMask(const VPBasicBlock *Src, const VPBasicBlock *Dst) const { | ||||||
|
@@ -226,6 +231,20 @@ void VPPredicator::createSwitchEdgeMasks(VPInstruction *SI) { | |||||
setEdgeMask(Src, DefaultDst, DefaultMask); | ||||||
} | ||||||
|
||||||
VPValue *VPPredicator::findCommonEdgeMask(const VPWidenPHIRecipe *PhiR) const { | ||||||
using namespace llvm::VPlanPatternMatch; | ||||||
VPValue *EdgeMask = getEdgeMask(PhiR->getIncomingBlock(0), PhiR->getParent()); | ||||||
VPValue *CommonEdgeMask; | ||||||
if (!EdgeMask || | ||||||
!match(EdgeMask, m_LogicalAnd(m_VPValue(CommonEdgeMask), m_VPValue()))) | ||||||
return nullptr; | ||||||
for (unsigned In = 1; In < PhiR->getNumIncoming(); In++) | ||||||
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.
Suggested change
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 seq is used very often in the loop vectorizer. Should we open up a separate patch to look at refactoring things to use it? 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. It's okay to just set an example in this instance: contributors will see it, and follow; a refactoring follow-on is optional. 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. Take our clang-format policy for instance: it's okay to have a mostly unformatted file, but we require that our changes are formatted. 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. Yes but I'm not sure if using seq for regular for loops is part of the loop vectorizer code style to begin with. I'm aware it's used a lot throughout SLP but I think it would be good to be consistent throughout the loop vectorizer. 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 (!match(getEdgeMask(PhiR->getIncomingBlock(In), PhiR->getParent()), | ||||||
artagnon marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
m_LogicalAnd(m_Specific(CommonEdgeMask), m_VPValue()))) | ||||||
return nullptr; | ||||||
return CommonEdgeMask; | ||||||
} | ||||||
|
||||||
void VPPredicator::convertPhisToBlends(VPBasicBlock *VPBB) { | ||||||
SmallVector<VPWidenPHIRecipe *> Phis; | ||||||
for (VPRecipeBase &R : VPBB->phis()) | ||||||
|
@@ -237,6 +256,7 @@ void VPPredicator::convertPhisToBlends(VPBasicBlock *VPBB) { | |||||
// be duplications since this is a simple recursive scan, but future | ||||||
// optimizations will clean it up. | ||||||
|
||||||
VPValue *CommonEdgeMask = findCommonEdgeMask(PhiR); | ||||||
SmallVector<VPValue *, 2> OperandsWithMask; | ||||||
unsigned NumIncoming = PhiR->getNumIncoming(); | ||||||
for (unsigned In = 0; In < NumIncoming; In++) { | ||||||
artagnon marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
@@ -249,6 +269,16 @@ void VPPredicator::convertPhisToBlends(VPBasicBlock *VPBB) { | |||||
"Distinct incoming values with one having a full mask"); | ||||||
break; | ||||||
} | ||||||
|
||||||
// If all incoming blocks share a common edge, remove it from the mask. | ||||||
if (CommonEdgeMask) { | ||||||
using namespace llvm::VPlanPatternMatch; | ||||||
VPValue *X; | ||||||
if (match(EdgeMask, | ||||||
m_LogicalAnd(m_Specific(CommonEdgeMask), m_VPValue(X)))) | ||||||
EdgeMask = X; | ||||||
} | ||||||
|
||||||
OperandsWithMask.push_back(EdgeMask); | ||||||
} | ||||||
PHINode *IRPhi = cast_or_null<PHINode>(PhiR->getUnderlyingValue()); | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.
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 would probably hoist the 'using namespace' (and the other instance).
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.
Do you mean hoist it outside the function? I'm not 100% opposed but I don't think we do that anywhere else in LLVM. I guess its to prevent clashes with the other LLVM IR pattern matcher