-
Notifications
You must be signed in to change notification settings - Fork 2.7k
JIT: cleanup redundant exact type tests #27397
JIT: cleanup redundant exact type tests #27397
Conversation
Generate exact type assertions from exact type tests in the IR. Look for these assertions and set the value number for RELOPs with known outcomes. Process JTRUE nodes in the main assertion prop optimization loop. This combination of changes removes residual exact type tests from cast expansions when they are anticipated by user inserted exact type tests, as in: ```C# if (b.GetType() == typeof(D)) { return (D)b; } ``` Closes #14471.
@dotnet/jit-contrib PTAL diffs:
on an example like the one in #14471, we now generate: G_M62121_IG01:
G_M62121_IG02:
mov rax, 0xD1FFAB1E
cmp qword ptr [rcx], rax
jne SHORT G_M62121_IG05
G_M62121_IG03: ;; bbWeight=0.50
mov rax, rcx
G_M62121_IG04: ;; bbWeight=0.50
ret
G_M62121_IG05: ;; bbWeight=0.50
xor rax, rax
G_M62121_IG06: ;; bbWeight=0.50
ret
|
src/jit/assertionprop.cpp
Outdated
{ | ||
GenTreeIndir* indir = op1->AsIndir(); | ||
|
||
if (!indir->HasIndex() && (indir->Offset() == 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should use GenTreeIndir
's HasIndex
, Base
and Offset
. These are practically meaningless before lowering because they depend on containment and address modes. Before lowering:
HasIndex()
is always falseBase()
is justAddr()
Offset()
is always 0
Basically these shouldn't exist onGenTreeIndir
, they're misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I can remove these.
@@ -3188,11 +3210,21 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen | |||
{ | |||
op1->ChangeOperConst(GT_CNS_INT); | |||
op1->AsIntCon()->gtIconVal = vnStore->ConstantValue<int>(vnCns); | |||
|
|||
if (vnStore->IsVNHandle(vnCns)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this case happens only on 32 bit targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also considering not propagating handle constants into trees as they're generally not foldable and end up producing large immediates.
{ | ||
// A GT_TRUE is always the last node in a tree, so we can break here | ||
assert((tree->gtNext == nullptr) && (stmt->GetNextStmt() == nullptr)); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, can't remember exactly why I added this early exit for JTRUE
nodes. Most likely for consistency with the assertion gen loop that has special handling for JTRUE
. Here the only potentially unwanted side effect of not exiting seems to be setting the wrong bit in assertions
. But since it's the last node of the block it doesn't matter...
@dotnet/jit-contrib anyone want to take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good, Left some comments
if (!tree->OperIs(GT_EQ, GT_NE)) | ||
{ | ||
return nullptr; | ||
} | ||
|
||
if (op1->gtOper != GT_LCL_VAR) | ||
// Bail out if tree is not side effect free. | ||
if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe the problem that you saw here when GTF_SIDE_EFFECT was true.
Did this change cause any current Asm diffs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No diffs from the previous version. I added this as a precaution, to parallel what we do above for the =/!= 0
case, since the simplification below will not preserve side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling of OAK_NOT_EQUAL for GT_IND not consistent with comments
src/jit/assertionprop.cpp
Outdated
*/ | ||
//------------------------------------------------------------------------ | ||
// optGlobalAssertionIsEqualOrNotEqual: Look for an assertion in the specified | ||
// set that is one of op1 == op1, op1 != op2, *op1 == op2, *op1 != op2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below only allows for *op1 == op2
, not *op1 != op2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Meant just to handle the equality case.
|
||
// Look for matching exact type assertions based on vtable accesses | ||
if ((curAssertion->assertionKind == OAK_EQUAL) && (curAssertion->op1.kind == O1K_EXACT_TYPE) && | ||
op1->OperIs(GT_IND)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't allow OAK_NOT_EQUAL
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right -- let me update the comment.
Generate exact type assertions from exact type tests in the IR. Look for these
assertions and set the value number for RELOPs with known outcomes. Process
JTRUE nodes in the main assertion prop optimization loop.
This combination of changes removes residual exact type tests from cast
expansions when they are anticipated by user inserted exact type tests, as in:
Closes #14471.