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.

Sign up for GitHub

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

Remove unnecessary GcInfoEncoder::DoNotTrackInPartiallyInterruptible #113078

Merged
merged 3 commits into from
Mar 4, 2025
+6 −102
Merged

Remove unnecessary GcInfoEncoder::DoNotTrackInPartiallyInterruptible #113078

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f6d5b6
Test for untracked slots in partially interruptible methods
jakobbotsch Mar 3, 2025
eefa928
Remove unnecessary check
jakobbotsch Mar 3, 2025
16ece14
Run superpmi on gcinfo changes
jakobbotsch Mar 3, 2025
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
102 changes: 4 additions & 98 deletions src/coreclr/gcinfo/gcinfoencoder.cpp
View file
Original file line number Diff line number Diff line change
@@ -909,97 +909,6 @@ void GcInfoEncoder::FinalizeSlotIds()
#endif
}

#ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED

// tells whether a slot cannot contain an object reference
// at call instruction or right after returning
bool GcInfoEncoder::DoNotTrackInPartiallyInterruptible(GcSlotDesc &slotDesc)
Copy link
Member

Choose a reason for hiding this comment

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

I think this was an optimization in the past to filter out volatile registers early.
It also claims to filter out things that cannot possibly contain a ref - but why would that even come as an input? The filtering probably did not help much in this regard.

Now that we record everything that is alive, the filtering is pointless indeed.

{
#if defined(TARGET_ARM)

_ASSERTE( m_SizeOfStackOutgoingAndScratchArea != (UINT32)-1 );
if(slotDesc.IsRegister())
{
int regNum = (int) slotDesc.Slot.RegisterNumber;
_ASSERTE(regNum >= 0 && regNum <= 14);
_ASSERTE(regNum != 13); // sp

return ((regNum <= 3) || (regNum >= 12)) // R12 is volatile and SP/LR can't contain objects around calls
&& regNum != 0 // R0 can contain return value
;
}
else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) &&
((UINT32)slotDesc.Slot.Stack.SpOffset < m_SizeOfStackOutgoingAndScratchArea))
{
return TRUE;
}
else
return FALSE;

#elif defined(TARGET_ARM64)

_ASSERTE(m_SizeOfStackOutgoingAndScratchArea != (UINT32)-1);
if (slotDesc.IsRegister())
{
int regNum = (int)slotDesc.Slot.RegisterNumber;
_ASSERTE(regNum >= 0 && regNum <= 30);
_ASSERTE(regNum != 18);

return (regNum <= 17 || regNum >= 29) // X0 through X17 are scratch, FP/LR can't be used for objects around calls
&& regNum != 0 // X0 can contain return value
&& regNum != 1 // X1 can contain return value
;
}
else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) &&
((UINT32)slotDesc.Slot.Stack.SpOffset < m_SizeOfStackOutgoingAndScratchArea))
{
return TRUE;
}
else
return FALSE;

#elif defined(TARGET_AMD64)

_ASSERTE( m_SizeOfStackOutgoingAndScratchArea != (UINT32)-1 );
if(slotDesc.IsRegister())
{
int regNum = (int) slotDesc.Slot.RegisterNumber;
_ASSERTE(regNum >= 0 && regNum <= 16);
_ASSERTE(regNum != 4); // rsp

UINT16 PreservedRegMask =
(1 << 3) // rbx
| (1 << 5) // rbp
#ifndef UNIX_AMD64_ABI
| (1 << 6) // rsi
| (1 << 7) // rdi
#endif // UNIX_AMD64_ABI
| (1 << 12) // r12
| (1 << 13) // r13
| (1 << 14) // r14
| (1 << 15) // r15
| (1 << 0) // rax - may contain return value
#ifdef UNIX_AMD64_ABI
| (1 << 2) // rdx - may contain return value
#endif
;

return !(PreservedRegMask & (1 << regNum));
}
else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) &&
((UINT32)slotDesc.Slot.Stack.SpOffset < m_SizeOfStackOutgoingAndScratchArea))
{
return TRUE;
}
else
return FALSE;

#else
return FALSE;
#endif
}
#endif // PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED

void GcInfoEncoder::Build()
{
#ifdef _DEBUG
@@ -1389,14 +1298,11 @@ void GcInfoEncoder::Build()
else
{
UINT32 slotIndex = pCurrent->SlotId;
if(!DoNotTrackInPartiallyInterruptible(m_SlotTable[slotIndex]))
{
BYTE becomesLive = pCurrent->BecomesLive;
_ASSERTE((liveState.ReadBit(slotIndex) && !becomesLive)
|| (!liveState.ReadBit(slotIndex) && becomesLive));
BYTE becomesLive = pCurrent->BecomesLive;
_ASSERTE((liveState.ReadBit(slotIndex) && !becomesLive)
|| (!liveState.ReadBit(slotIndex) && becomesLive));

liveState.WriteBit(slotIndex, becomesLive);
}
liveState.WriteBit(slotIndex, becomesLive);
pCurrent++;
}
}
4 changes: 0 additions & 4 deletions src/coreclr/inc/gcinfoencoder.h
View file
Original file line number Diff line number Diff line change
@@ -542,10 +542,6 @@ class GcInfoEncoder
void SizeofSlotStateVarLengthVector(const BitArray& vector, UINT32 baseSkip, UINT32 baseRun, UINT32 * pSizeofSimple, UINT32 * pSizeofRLE, UINT32 * pSizeofRLENeg);
UINT32 WriteSlotStateVarLengthVector(BitStreamWriter &writer, const BitArray& vector, UINT32 baseSkip, UINT32 baseRun);

#ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED
bool DoNotTrackInPartiallyInterruptible(GcSlotDesc &slot);
#endif // PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED

// Assumes that "*ppTransitions" is has size "numTransitions", is sorted by CodeOffset then by SlotId,
// and that "*ppEndTransitions" points one beyond the end of the array. If "*ppTransitions" contains
// any dead/live transitions pairs for the same CodeOffset and SlotID, removes those, by allocating a