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

Improvements for null check folding. #1735

Merged
merged 1 commit into from
Jan 17, 2020
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
47 changes: 47 additions & 0 deletions src/coreclr/format.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
index a71c325ff48..c0569355c89 100644
--- a/src/coreclr/src/jit/importer.cpp
+++ b/src/coreclr/src/jit/importer.cpp
@@ -15184,41 +15184,41 @@ void Compiler::impImportBlockCode(BasicBlock* block)
info.compCompHnd->compareTypesForEquality(resolvedToken.hClass, clsHnd);

if (compare == TypeCompareState::Must)
{
JITDUMP("\nOptimizing %s (%s) -- type test will succeed\n",
opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY", eeGetClassName(clsHnd));

// For UNBOX, null check (if necessary), and then leave the box payload byref on the stack.
if (opcode == CEE_UNBOX)
{
GenTree* cloneOperand;
op1 = impCloneExpr(op1, &cloneOperand, NO_CLASS_HANDLE, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("optimized unbox clone"));

GenTree* boxPayloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL);
GenTree* boxPayloadAddress =
gtNewOperNode(GT_ADD, TYP_BYREF, cloneOperand, boxPayloadOffset);
GenTree* nullcheck = gtNewOperNode(GT_NULLCHECK, TYP_I_IMPL, op1);
block->bbFlags |= BBF_HAS_NULLCHECK;
optMethodFlags |= OMF_HAS_NULLCHECK;
- GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);
+ GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);
impPushOnStack(result, tiRetVal);
break;
}

// For UNBOX.ANY load the struct from the box payload byref (the load will nullcheck)
assert(opcode == CEE_UNBOX_ANY);
GenTree* boxPayloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL);
GenTree* boxPayloadAddress = gtNewOperNode(GT_ADD, TYP_BYREF, op1, boxPayloadOffset);
impPushOnStack(boxPayloadAddress, tiRetVal);
oper = GT_OBJ;
goto OBJ;
}
else
{
JITDUMP("\nUnable to optimize %s -- can't resolve type comparison\n",
opcode == CEE_UNBOX ? "UNBOX" : "UNBOX.ANY");
}
}
else
{
4 changes: 4 additions & 0 deletions src/coreclr/src/jit/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,10 @@ void BasicBlock::dspFlags()
{
printf("newobj ");
}
if (bbFlags & BBF_HAS_NULLCHECK)
{
printf("nullcheck ");
}
#if defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
if (bbFlags & BBF_FINALLY_TARGET)
{
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/src/jit/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ struct BasicBlock : private LIR::Range

#define BBF_COMPACT_UPD \
(BBF_CHANGED | BBF_GC_SAFE_POINT | BBF_HAS_JMP | BBF_NEEDS_GCPOLL | BBF_HAS_IDX_LEN | BBF_BACKWARD_JUMP | \
BBF_HAS_NEWARRAY | BBF_HAS_NEWOBJ)
BBF_HAS_NEWARRAY | BBF_HAS_NEWOBJ | BBF_HAS_NULLCHECK)
Copy link
Member

Choose a reason for hiding this comment

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

Curious how you found these missing propagation bits... did you experiment with temporarily making optEarlyPropFor... always return true? (if not, may be worth a try).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I just saw some null checks not getting removed in some diffs and tracked that down to the missing flags. Good idea to try with optDoEarlyPropFor... returning always true. Will try tomorrow.

Copy link
Member Author

@erozenfeld erozenfeld Jan 16, 2020

Choose a reason for hiding this comment

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

I added missing OMF_HAS_NULLCHECK and BBF_HAS_NULLCHECK in a couple of places but they didn't result in any new diffs. Changing optDoEarlyPropFor... to always return true does result in diffs but they don't seem to be related to nullchecks. I will follow up outside of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Apparently it was me who was forgetting to add these....


// Flags a block should not have had before it is split.

Expand All @@ -481,14 +481,14 @@ struct BasicBlock : private LIR::Range

// Flags gained by the bottom block when a block is split.
// Note, this is a conservative guess.
// For example, the bottom block might or might not have BBF_HAS_NEWARRAY,
// but we assume it has BBF_HAS_NEWARRAY.
// For example, the bottom block might or might not have BBF_HAS_NEWARRAY or BBF_HAS_NULLCHECK,
// but we assume it has BBF_HAS_NEWARRAY and BBF_HAS_NULLCHECK.

// TODO: Should BBF_RUN_RARELY be added to BBF_SPLIT_GAINED ?

#define BBF_SPLIT_GAINED \
(BBF_DONT_REMOVE | BBF_HAS_LABEL | BBF_HAS_JMP | BBF_BACKWARD_JUMP | BBF_HAS_IDX_LEN | BBF_HAS_NEWARRAY | \
BBF_PROF_WEIGHT | BBF_HAS_NEWOBJ | BBF_KEEP_BBJ_ALWAYS | BBF_CLONED_FINALLY_END)
BBF_PROF_WEIGHT | BBF_HAS_NEWOBJ | BBF_KEEP_BBJ_ALWAYS | BBF_CLONED_FINALLY_END | BBF_HAS_NULLCHECK)

#ifndef __GNUC__ // GCC doesn't like C_ASSERT at global scope
static_assert_no_msg((BBF_SPLIT_NONEXIST & BBF_SPLIT_LOST) == 0);
Expand Down
16 changes: 13 additions & 3 deletions src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -6396,17 +6396,27 @@ class Compiler
OPK_NULLCHECK
};

typedef JitHashTable<unsigned, JitSmallPrimitiveKeyFuncs<unsigned>, GenTree*> LocalNumberToNullCheckTreeMap;

bool gtIsVtableRef(GenTree* tree);
GenTree* getArrayLengthFromAllocation(GenTree* tree);
GenTree* getObjectHandleNodeFromAllocation(GenTree* tree);
GenTree* optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropKind valueKind, int walkDepth);
GenTree* optPropGetValue(unsigned lclNum, unsigned ssaNum, optPropKind valueKind);
GenTree* optEarlyPropRewriteTree(GenTree* tree);
GenTree* optEarlyPropRewriteTree(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap);
bool optDoEarlyPropForBlock(BasicBlock* block);
bool optDoEarlyPropForFunc();
void optEarlyProp();
void optFoldNullCheck(GenTree* tree);
bool optCanMoveNullCheckPastTree(GenTree* tree, bool isInsideTry);
void optFoldNullCheck(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap);
GenTree* optFindNullCheckToFold(GenTree* tree, LocalNumberToNullCheckTreeMap* nullCheckMap);
bool optIsNullCheckFoldingLegal(GenTree* tree,
GenTree* nullCheckTree,
GenTree** nullCheckParent,
Statement** nullCheckStmt);
bool optCanMoveNullCheckPastTree(GenTree* tree,
unsigned nullCheckLclNum,
bool isInsideTry,
bool checkSideEffectSummary);

#if ASSERTION_PROP
/**************************************************************************
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/compmemkind.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ CompMemKindMacro(ObjectAllocator)
CompMemKindMacro(VariableLiveRanges)
CompMemKindMacro(ClassLayout)
CompMemKindMacro(TailMergeThrows)
CompMemKindMacro(EarlyProp)
//clang-format on

#undef CompMemKindMacro
Loading