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

Fold "cns"[cns] for ROS<char> #78593

Merged
merged 15 commits into from
Nov 21, 2022
17 changes: 17 additions & 0 deletions src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -2518,6 +2518,23 @@ class ICorStaticInfo
CORINFO_OBJECT_HANDLE objPtr
) = 0;

//------------------------------------------------------------------------------
// getStringChar: returns char at the given index if the given object handle
// represents String and index is not out of bounds.
//
// Arguments:
// strObj - object handle
// index - index of the char to return
// value - output char
//
// Return Value:
// Returns true if value was successfully obtained
//
virtual bool getStringChar(
CORINFO_OBJECT_HANDLE strObj,
int index,
uint16_t* value) = 0;

//------------------------------------------------------------------------------
// getObjectType: obtains type handle for given object
//
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/inc/icorjitinfoimpl_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,11 @@ CORINFO_OBJECT_HANDLE getRuntimeTypePointer(
bool isObjectImmutable(
CORINFO_OBJECT_HANDLE objPtr) override;

bool getStringChar(
CORINFO_OBJECT_HANDLE strObj,
int index,
uint16_t* value) override;

CORINFO_CLASS_HANDLE getObjectType(
CORINFO_OBJECT_HANDLE objPtr) override;

Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID;
#define GUID_DEFINED
#endif // !GUID_DEFINED

constexpr GUID JITEEVersionIdentifier = { /* e3c98d96-6e67-459a-9750-ebe799cfb788 */
0xe3c98d96,
0x6e67,
0x459a,
{0x97, 0x50, 0xeb, 0xe7, 0x99, 0xcf, 0xb7, 0x88}
constexpr GUID JITEEVersionIdentifier = { /* 6f8fc8da-0a65-4054-be5d-505108298910 */
0x6f8fc8da,
0xa65,
0x4054,
{0xbe, 0x5d, 0x50, 0x51, 0x8, 0x29, 0x89, 0x10}
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/ICorJitInfo_names_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ DEF_CLR_API(getBoxHelper)
DEF_CLR_API(getUnBoxHelper)
DEF_CLR_API(getRuntimeTypePointer)
DEF_CLR_API(isObjectImmutable)
DEF_CLR_API(getStringChar)
DEF_CLR_API(getObjectType)
DEF_CLR_API(getReadyToRunHelper)
DEF_CLR_API(getReadyToRunDelegateCtorHelper)
Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,17 @@ bool WrapICorJitInfo::isObjectImmutable(
return temp;
}

bool WrapICorJitInfo::getStringChar(
CORINFO_OBJECT_HANDLE strObj,
int index,
uint16_t* value)
{
API_ENTER(getStringChar);
bool temp = wrapHnd->getStringChar(strObj, index, value);
API_LEAVE(getStringChar);
return temp;
}

CORINFO_CLASS_HANDLE WrapICorJitInfo::getObjectType(
CORINFO_OBJECT_HANDLE objPtr)
{
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4827,6 +4827,8 @@ class Compiler
void fgValueNumberFieldStore(
GenTree* storeNode, GenTree* baseAddr, FieldSeq* fieldSeq, ssize_t offset, unsigned storeSize, ValueNum value);

bool fgValueNumberConstStringElemLoad(GenTreeIndir* tree);

// Compute the value number for a byref-exposed load of the given type via the given pointerVN.
ValueNum fgValueNumberByrefExposedLoad(var_types type, ValueNum pointerVN);

Expand Down Expand Up @@ -4874,7 +4876,7 @@ class Compiler
// Assumes that all inputs to "tree" have had value numbers assigned; assigns a VN to tree.
// (With some exceptions: the VN of the lhs of an assignment is assigned as part of the
// assignment.)
void fgValueNumberTree(GenTree* tree);
void fgValueNumberTree(GenTree* tree, Statement* stmt);

void fgValueNumberAssignment(GenTreeOp* tree);

Expand Down
102 changes: 100 additions & 2 deletions src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7884,7 +7884,7 @@ void Compiler::fgValueNumberBlock(BasicBlock* blk)
{
// Set up ambient var referring to current tree.
compCurTree = tree;
fgValueNumberTree(tree);
fgValueNumberTree(tree, stmt);
compCurTree = nullptr;
}

Expand Down Expand Up @@ -8492,7 +8492,98 @@ void Compiler::fgValueNumberSsaVarDef(GenTreeLclVarCommon* lcl)
}
}

void Compiler::fgValueNumberTree(GenTree* tree)
//------------------------------------------------------------------------
// fgValueNumberConstStringElemLoad: Try to match "cns_str"[cns_index] tree
// and apply a constant VN representing given char at cns_index in that string.
//
// Arguments:
// tree - the GT_IND node
//
// Return Value:
// true if the pattern was recognized and a new VN is assigned
//
bool Compiler::fgValueNumberConstStringElemLoad(GenTreeIndir* tree)
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
ValueNum addrVN = tree->gtGetOp1()->gtVNPair.GetLiberal();
VNFuncApp funcApp;
if (!varTypeIsShort(tree) || !vnStore->GetVNFunc(addrVN, &funcApp))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we interested in TYP_SHORT (signed) trees, since I assume this is a TP heuristic?

This shouldn't have to recompute addrVN + GetVNFunc, since the caller already did 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.

@SingleAccretion can you point me where exactly caller already did it? it seems like other paths also compute it

else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic))
{
fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]);
offset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[2]);
// Note VNF_PtrToStatic statics are currently always "simple".
fgValueNumberFieldLoad(tree, /* baseAddr */ nullptr, fldSeq, offset);
}
else if (tree->OperIs(GT_IND) && fgValueNumberConstLoad(tree->AsIndir()))
{
// VN is assigned inside fgValueNumberConstLoad
}
else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToArrElem))
{
fgValueNumberArrayElemLoad(tree, &funcApp);
}
.

Changed to TYP_SHORT (yes, it was a TP fast-out path)

Copy link
Contributor

Choose a reason for hiding this comment

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

can you point me where exactly caller already did it? it seems like other paths also compute it

else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) computes it. It would be good to change this to compute the thing once like ASG numbering does.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mind if I leave it as is, I've just pushed a change to extract vnfunc only after TP-oriented checks. I tried to re-organize code to save these two existing lookups and didn't like the outcome, feel free to file a PR to clean it up

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

{
return false;
}

// Is given VN representing a frozen object handle
auto isCnsObjHandle = [](ValueNumStore* vnStore, ValueNum vn, CORINFO_OBJECT_HANDLE* handle) -> bool {
if (vnStore->IsVNHandle(vn) && (vnStore->GetHandleFlags(vn) == GTF_ICON_OBJ_HDL))
{
const size_t obj = vnStore->CoercedConstantValue<size_t>(vn);
*handle = reinterpret_cast<CORINFO_OBJECT_HANDLE>(obj);
return true;
}
return false;
};

CORINFO_OBJECT_HANDLE objHandle = nullptr;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
int index = -1;

// First, let see if we have PtrToArrElem
ValueNum addr = funcApp.m_args[0];
if (funcApp.m_func == VNF_PtrToArrElem)
{
ValueNum arrVN = funcApp.m_args[1];
ValueNum inxVN = funcApp.m_args[2];
ssize_t offset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[3]);

if (isCnsObjHandle(vnStore, arrVN, &objHandle) && offset == 0 && vnStore->IsVNConstant(inxVN))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
index = (int)vnStore->CoercedConstantValue<size_t>(inxVN);
}
}
else if (((genTreeOps)funcApp.m_func == GT_ADD) && vnStore->IsVNConstant(funcApp.m_args[0]))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
// Here we try to match these two patterns:
//
// 1) (objHandle + firstCharOffset)
// 2) ((objHandle + firstCharOffset) + byteIndex)
//
size_t dataOffset = 0;
if (vnStore->IsVNConstant(funcApp.m_args[1]) && isCnsObjHandle(vnStore, funcApp.m_args[0], &objHandle))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
dataOffset = vnStore->CoercedConstantValue<size_t>(funcApp.m_args[1]);
}
else
{
assert(vnStore->IsVNConstant(funcApp.m_args[0]));
const size_t byteDataIndex = vnStore->CoercedConstantValue<size_t>(funcApp.m_args[0]);
if (vnStore->GetVNFunc(funcApp.m_args[1], &funcApp) && ((genTreeOps)funcApp.m_func == GT_ADD) &&
vnStore->IsVNConstant(funcApp.m_args[1]) && isCnsObjHandle(vnStore, funcApp.m_args[0], &objHandle))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
dataOffset = vnStore->CoercedConstantValue<size_t>(funcApp.m_args[1]) + byteDataIndex;
}
}

// Convert dataOffset (that also includes offset to firstChar field) into char index
if ((dataOffset >= OFFSETOF__CORINFO_String__chars) && ((dataOffset % 2) == 0))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
index = (int)(dataOffset - OFFSETOF__CORINFO_String__chars) / 2;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
}
}

if ((objHandle != nullptr) && (index >= 0))
{
assert(objHandle != nullptr);
USHORT charValue;
if (info.compCompHnd->getStringChar(objHandle, index, &charValue))
{
JITDUMP("Folding \"cns_str\"[%d] into %u\n", index, (unsigned)charValue);
// NOTE: we need to sign-extend it here in case if it's a negative Int16
tree->gtVNPair.SetBoth(vnStore->VNForIntCon((SHORT)charValue));
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
}
return false;
}

void Compiler::fgValueNumberTree(GenTree* tree, Statement* stmt)
{
genTreeOps oper = tree->OperGet();
var_types typ = tree->TypeGet();
Expand Down Expand Up @@ -8743,6 +8834,13 @@ void Compiler::fgValueNumberTree(GenTree* tree)
// Note VNF_PtrToStatic statics are currently always "simple".
fgValueNumberFieldLoad(tree, /* baseAddr */ nullptr, fldSeq, offset);
}
else if (tree->OperIs(GT_IND) && fgValueNumberConstStringElemLoad(tree->AsIndir()))
{
tree->gtFlags |= GTF_IND_NONFAULTING | GTF_IND_INVARIANT;
tree->gtFlags &= ~GTF_EXCEPT;
// Update side-effect flags of parent trees if needed
gtUpdateStmtSideEffects(stmt);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not right to update side effects in VN like that. The problem this solves should be solved in some other way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SingleAccretion Any recommendation and what's wrong with it? Unfortunately, I don't see a better option and this is required for this PR.
Alternative option I see is to define a new VN with "known-non-null" property and then fold it later e.g. at assertprop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any recommendation

From the discussion yesterday - did making VN-based constant propagation run in post-order not work?

what's wrong with it

It's not in VN's contract to modify IR like this. Certainly, if you allow this, all sorts of things become possible (for example, here we would just bash the node to a CNS_INT directly).

Copy link
Member Author

@EgorBo EgorBo Nov 20, 2022

Choose a reason for hiding this comment

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

Thanks! Reverted the change, the suggestion about post-order works but I decided to postpone it to a separate PR so I can see if it worth the effort around TP and diffs (and planned a few experiments around that) so this PR does fold all the patterns I wanted it to, but might leave nullchecks in some cases.

}
else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToArrElem))
{
fgValueNumberArrayElemLoad(tree, &funcApp);
Expand Down
Loading