Uh oh!
There was an error while loading. Please reload this page.
arm64: Reuse SVE mask constants in LSRA - #131309
Conversation
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| #if defined(TARGET_ARM64) && defined(FEATURE_MASKED_HW_INTRINSICS) | ||
| struct SveMaskIntervalEntry | ||
| { | ||
| GenTree* tree; | ||
| Interval* interval; | ||
| SveMaskIntervalEntry* next; | ||
| }; | ||
| SveMaskIntervalEntry* reusableSveMaskIntervals = nullptr; | ||
| #endif |
There was a problem hiding this comment.
I was not expecting to see anything like this. What makes this needed over the existing constant reuse mechanism that LSRA already has? E.g. members like LinearScan::m_RegistersWithConstants and LinearScan::getMatchingConstants.
Did you figure out why the existing mechanism does not handle the cases this tries to handle? It would be good to understand that first.
There was a problem hiding this comment.
I removed the shared-interval mechanism. The existing LSRA reuse machinery already handles GT_CNS_MSK, but SVE ptrue values are often represented as GT_HWINTRINSIC and were not marked as constants. Therefore isMatchingConstant did not consider them reusable. The updated change now uses the existing mechanism.
There was a problem hiding this comment.
We import TrueMask/FalseMask as GT_CNS_MASK where possible.
If the pattern is in a variable, then we have to import the HWINTRINSIC - but that's not something we can optimise in the PR as we don't know the pattern.
Then there are all the "all ptrue" we introduce during lowering. They are also added as GT_CNS_MASK nodes now too.
So, yes, this most likely doesn't need to check for HWINTRINSIC any more.
Also, this PR might need some JitUseScalableVectorT() checks now too
There was a problem hiding this comment.
I've done that
| #if defined(TARGET_ARM64) && defined(FEATURE_MASKED_HW_INTRINSICS) | ||
| if (areMatchingSveMaskConstants(refPosition->treeNode, otherTreeNode)) | ||
| { | ||
| return true; | ||
| } | ||
| #endif | ||
There was a problem hiding this comment.
This should not be needed, can you please fix the code called in the GT_CNS_MSK case below if that isn't already sufficient?
jonathandavies-arm
commented
Aug 4, 2026
An earlier implementation preserved SVE mask constants across certain basic-block boundaries: BasicBlock* nextBlock = getNextBlock();
bool preserveMaskConstants = false;
preserveMaskConstants =
m_compiler->opts.OptimizationEnabled() &&
(nextBlock != nullptr) &&
(nextBlock->GetUniquePred(m_compiler) == currentBlock) &&
!blockInfo[nextBlock->bbNum].hasEHBoundaryIn &&
!blockInfo[currentBlock->bbNum].hasEHBoundaryOut;
...
if (preserveMaskConstants && assignedInterval->isConstant &&
varTypeIsMask(assignedInterval->registerType))
{
setConstantReg(reg, assignedInterval->registerType);
continue;
}Should LSRA allow constant registers to survive a block boundary when the value is not live into the successor? Is the current behavior (clearing constant-register state at every block boundary and rematerializing constants in each basic block) the intended approach, or should LSRA support preserving these constants across eligible block boundaries? |
This is the ptrue reuse work done in LSRA instead of a new pass.