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

Improve constant folding for some frozen objects #86318

Merged
merged 4 commits into from
May 17, 2023
Merged

Improve constant folding for some frozen objects #86318

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb1a0fb
Make jit fold bool.parse("true")
EgorBo May 16, 2023
546f545
Fix typo
EgorBo May 16, 2023
ca93999
Use target_ssize_t
EgorBo May 16, 2023
71439a2
Update valuenum.cpp
EgorBo May 16, 2023
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
38 changes: 21 additions & 17 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5796,6 +5796,11 @@ bool ValueNumStore::IsVNConstant(ValueNum vn)
}
}

bool ValueNumStore::IsVNConstantNonHandle(ValueNum vn)
{
return IsVNConstant(vn) && !IsVNHandle(vn);
}

bool ValueNumStore::IsVNInt32Constant(ValueNum vn)
{
if (!IsVNConstant(vn))
Expand Down Expand Up @@ -10379,38 +10384,37 @@ static bool GetObjectHandleAndOffset(ValueNumStore* vnStore,
ssize_t* byteOffset,
CORINFO_OBJECT_HANDLE* pObj)
{

if (!tree->gtVNPair.BothEqual())
{
return false;
}

ValueNum treeVN = tree->gtVNPair.GetLiberal();
VNFuncApp funcApp;
if (vnStore->GetVNFunc(treeVN, &funcApp) && (funcApp.m_func == (VNFunc)GT_ADD))
size_t offset = 0;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
while (vnStore->GetVNFunc(treeVN, &funcApp) && (funcApp.m_func == (VNFunc)GT_ADD))
{
// [objHandle + offset]
if (vnStore->IsVNObjHandle(funcApp.m_args[0]) && vnStore->IsVNConstant(funcApp.m_args[1]))
if (vnStore->IsVNConstantNonHandle(funcApp.m_args[0]) && (vnStore->TypeOfVN(funcApp.m_args[0]) == TYP_I_IMPL))
{
*pObj = vnStore->ConstantObjHandle(funcApp.m_args[0]);
*byteOffset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[1]);
Copy link
Member

@jakobbotsch jakobbotsch May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm surprised we haven't hit this assert with this previous code:

runtime/src/coreclr/jit/valuenum.h

Lines 1080 to 1099 in bda1f6c

T val1 = reinterpret_cast<T*>(c->m_defs)[offset];
T val2 = SafeGetConstantValue<T>(c, offset);
// Detect if there is a mismatch between the VN storage type and explicitly
// passed-in type T.
bool mismatch = false;
if (varTypeIsFloating(c->m_typ))
{
mismatch = (memcmp(&val1, &val2, sizeof(val1)) != 0);
}
else
{
mismatch = (val1 != val2);
}
if (mismatch)
{
assert(
!"Called ConstantValue<T>(vn), but type(T) != type(vn); Use CoercedConstantValue instead.");
}

I would expect it to hit for 64-bit to 32-bit crossgen2 compilations. It seems to indicate that this code is never hit for crossgen2 32-bit compilations? Is that expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect it to hit for 64-bit to 32-bit crossgen2 compilations.

That's correct - it will be hit if you replay a 32 bit collection with a cross-targeting Jit today. Evidently, we just don't have people (or machines) doing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

crossgen doesn't have frozen objects so this path indeed has never been executed, but we could see it on NativeAOT, but I'm not sure we cover the cross compilation case on CI for it

return true;
offset += vnStore->CoercedConstantValue<ssize_t>(funcApp.m_args[0]);
treeVN = funcApp.m_args[1];
}

// [offset + objHandle]
// TODO: Introduce a general helper to accumulate offsets for
// shapes such as (((X + CNS1) + CNS2) + CNS3) etc.
if (vnStore->IsVNObjHandle(funcApp.m_args[1]) && vnStore->IsVNConstant(funcApp.m_args[0]))
else if (vnStore->IsVNConstantNonHandle(funcApp.m_args[1]) &&
(vnStore->TypeOfVN(funcApp.m_args[1]) == TYP_I_IMPL))
{
*pObj = vnStore->ConstantObjHandle(funcApp.m_args[1]);
*byteOffset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[0]);
return true;
offset += vnStore->CoercedConstantValue<ssize_t>(funcApp.m_args[1]);
treeVN = funcApp.m_args[0];
}
else
{
return false;
}
}
else if (vnStore->IsVNObjHandle(treeVN))

if (vnStore->IsVNObjHandle(treeVN))
{
*pObj = vnStore->ConstantObjHandle(treeVN);
*byteOffset = 0;
*byteOffset = offset;
return true;
}
return false;
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/jit/valuenum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,9 +858,12 @@ class ValueNumStore
// Returns BasicBlock::MAX_LOOP_NUM if the given value number's loop nest is unknown or ill-defined.
BasicBlock::loopNumber LoopOfVN(ValueNum vn);

// Returns true iff the VN represents a (non-handle) constant.
// Returns true iff the VN represents a constant.
bool IsVNConstant(ValueNum vn);

// Returns true iff the VN represents a (non-handle) constant.
bool IsVNConstantNonHandle(ValueNum vn);

// Returns true iff the VN represents an integer constant.
bool IsVNInt32Constant(ValueNum vn);

Expand Down
11 changes: 9 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,14 @@ public static bool Parse(string value)
return Parse(value.AsSpan());
}

public static bool Parse(ReadOnlySpan<char> value) =>
TryParse(value, out bool result) ? result : throw new FormatException(SR.Format(SR.Format_BadBoolean, new string(value)));
public static bool Parse(ReadOnlySpan<char> value)
{
if (!TryParse(value, out bool result))
{
ThrowHelper.ThrowFormatException_BadBoolean(value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced throw with a throw helper to make it inlineable (at least for constant input)

}
return result;
}

// Determines whether a String represents true or false.
//
Expand Down Expand Up @@ -267,6 +273,7 @@ public static bool TryParse(ReadOnlySpan<char> value, out bool result)

return TryParseUncommon(value, out result);

[MethodImpl(MethodImplOptions.NoInlining)]
static bool TryParseUncommon(ReadOnlySpan<char> value, out bool result)
Copy link
Member Author

Choose a reason for hiding this comment

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

TryParseUncommon used to be inlineable in some cases (e.g. without pgo data)

{
// With "true" being 4 characters, even if we trim something from <= 4 chars,
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,12 @@ internal static void ThrowFormatException_NeedSingleChar()
throw new FormatException(SR.Format_NeedSingleChar);
}

[DoesNotReturn]
internal static void ThrowFormatException_BadBoolean(ReadOnlySpan<char> value)
{
throw new FormatException(SR.Format(SR.Format_BadBoolean, new string(value)));
}

[DoesNotReturn]
internal static void ThrowArgumentOutOfRangeException_PrecisionTooLarge()
{
Expand Down