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
Merged

Remove unnecessary GcInfoEncoder::DoNotTrackInPartiallyInterruptible #113078

Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions eng/pipelines/coreclr/superpmi-diffs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pr:
# so that this build can block dependency auto-updates (this build is currently ignored)
include:
- src/coreclr/jit/*
- src/coreclr/gcinfo/*
Copy link
Member

Choose a reason for hiding this comment

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

What are these changes for? (just curious)

Copy link
Member Author

Choose a reason for hiding this comment

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

superpmi-asmdiffs was not running for this change, even though the gcinfoencoder is statically linked into the JIT and affects its compilation results. This change is ensuring that superpmi-asmdiffs runs even on gcinfoencoder changes (so that we can see diffs).


variables:
- template: /eng/pipelines/common/variables.yml
Expand Down
1 change: 1 addition & 0 deletions eng/pipelines/coreclr/superpmi-replay.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pr:
paths:
include:
- src/coreclr/jit/*
- src/coreclr/gcinfo/*
- src/coreclr/tools/superpmi/*
exclude:
- src/coreclr/inc/jiteeversionguid.h
Expand Down
102 changes: 4 additions & 98 deletions src/coreclr/gcinfo/gcinfoencoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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++;
}
}
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/inc/gcinfoencoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading