Skip to content
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

Don't use VN when it's already stale #97943

Merged
merged 3 commits into from
Feb 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/jit/gcinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ GCInfo::WriteBarrierForm GCInfo::gcIsWriteBarrierCandidate(GenTreeStoreInd* stor
}

// Ignore any assignments of NULL.
GenTree* const data = store->Data()->gtSkipReloadOrCopy();
if ((data->GetVN(VNK_Liberal) == ValueNumStore::VNForNull()) || data->IsIntegralConst(0))
GenTree* const data = store->Data()->gtSkipReloadOrCopy()->gtEffectiveVal();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? GT_COMMA should not exist at this point, and if it did it is questionable to not skip reloads inside it (would cause issues during codegen).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, completely unnecessary, will remove as part of some other PR

if (data->IsIntegralConst(0))
{
return WBF_NoBarrier;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -4101,7 +4101,7 @@ enum GenTreeCallFlags : unsigned int
GTF_CALL_M_GUARDED_DEVIRT_CHAIN = 0x00080000, // this call is a candidate for chained guarded devirtualization
GTF_CALL_M_ALLOC_SIDE_EFFECTS = 0x00100000, // this is a call to an allocator with side effects
GTF_CALL_M_SUPPRESS_GC_TRANSITION = 0x00200000, // suppress the GC transition (i.e. during a pinvoke) but a separate GC safe point is required.
GTF_CALL_M_EXP_RUNTIME_LOOKUP = 0x00400000, // this call needs to be transformed into CFG for the dynamic dictionary expansion feature.
GTF_CALL_M_EXP_RUNTIME_LOOKUP = 0x00400000, // [DEBUG only] this call needs to be transformed into CFG for the dynamic dictionary expansion feature.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is debug only can you please move it to GenTreeCallDebugFlags?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was sort of unrelated to this PR, but good idea! One less bit occupied.

GTF_CALL_M_EXPANDED_EARLY = 0x00800000, // the Virtual Call target address is expanded and placed in gtControlExpr in Morph rather than in Lower
GTF_CALL_M_HAS_LATE_DEVIRT_INFO = 0x01000000, // this call has late devirtualzation info
GTF_CALL_M_LDVIRTFTN_INTERFACE = 0x02000000, // ldvirtftn on an interface type
Expand Down
52 changes: 9 additions & 43 deletions src/coreclr/jit/helperexpansion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,6 @@
#pragma hdrstop
#endif

// Obtain constant pointer from a tree
static void* GetConstantPointer(Compiler* comp, GenTree* tree)
{
void* cns = nullptr;
if (tree->gtEffectiveVal()->IsCnsIntOrI())
{
cns = (void*)tree->gtEffectiveVal()->AsIntCon()->IconValue();
}
else if (comp->vnStore->IsVNConstant(tree->gtVNPair.GetLiberal()))
{
cns = (void*)comp->vnStore->CoercedConstantValue<ssize_t>(tree->gtVNPair.GetLiberal());
}
return cns;
}

// Save expression to a local and append it as the last statement in exprBlock
static GenTree* SpillExpression(Compiler* comp, GenTree* expr, BasicBlock* exprBlock, DebugInfo& debugInfo)
{
Expand Down Expand Up @@ -186,14 +171,16 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stm
//
// type = call(genericCtx, signatureCns);
//
void* signature = GetConstantPointer(this, call->gtArgs.GetArgByIndex(1)->GetNode());
if (signature == nullptr)
const GenTree* signatureNode = call->gtArgs.GetArgByIndex(1)->GetNode();
if (!signatureNode->IsCnsIntOrI())
{
// Technically, it is possible (e.g. it was CSE'd and then VN was erased), but for Debug mode we
// want to catch such cases as we really don't want to emit just a fallback call - it's too slow
// We expect the signature to be a constant node here (it's marked as DONT_CSE)
// It's still correct if JIT decides to violate this assumption, but we really
// don't want it to happen, hence, the assert.
assert(!"can't restore signature argument value");
return false;
}
void* signature = reinterpret_cast<void*>(signatureNode->AsIntCon()->IconValue());

JITDUMP("Expanding runtime lookup for [%06d] in " FMT_BB ":\n", dspTreeID(call), block->bbNum)
DISPTREE(call)
Expand Down Expand Up @@ -1635,40 +1622,19 @@ bool Compiler::fgVNBasedIntrinsicExpansionForCall_ReadUtf8(BasicBlock** pBlock,

DebugInfo debugInfo = stmt->GetDebugInfo();

// Split block right before the call tree (this is a standard pattern we use in helperexpansion.cpp)
BasicBlock* prevBb = block;
GenTree** callUse = nullptr;
Statement* newFirstStmt = nullptr;
block = fgSplitBlockBeforeTree(block, stmt, call, &newFirstStmt, &callUse);
assert(prevBb != nullptr && block != nullptr);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change, just a clean up since SplitAtTreeAndReplaceItWithLocal does exactly that.

BasicBlock* prevBb;
const unsigned resultLclNum = SplitAtTreeAndReplaceItWithLocal(this, block, stmt, call, &prevBb, &block);

*pBlock = block;

// If we suddenly need to use these arguments, we'll have to reload them from the call
// after the split, so let's null them to prevent accidental use.
srcLen = nullptr;
srcPtr = nullptr;

// Block ops inserted by the split need to be morphed here since we are after morph.
// We cannot morph stmt yet as we may modify it further below, and the morphing
// could invalidate callUse
while ((newFirstStmt != nullptr) && (newFirstStmt != stmt))
{
fgMorphStmtBlockOps(block, newFirstStmt);
newFirstStmt = newFirstStmt->GetNextStmt();
}

// We don't need this flag anymore.
call->gtCallMoreFlags &= ~GTF_CALL_M_SPECIAL_INTRINSIC;

// Grab a temp to store the result.
// The result corresponds the number of bytes written to dstPtr (int32).
assert(call->TypeIs(TYP_INT));
const unsigned resultLclNum = lvaGrabTemp(true DEBUGARG("local for result"));
lvaTable[resultLclNum].lvType = TYP_INT;
*callUse = gtNewLclvNode(resultLclNum, TYP_INT);
fgMorphStmtBlockOps(block, stmt);
gtUpdateStmtSideEffects(stmt);

// srcLenU8 is the length of the string literal in chars (UTF16)
// but we're going to use the same value as the "bytesWritten" result in the fast path and in the length check.
GenTree* srcLenU8Node = gtNewIconNode(srcLenU8);
Expand Down
Loading