JIT: remove assert from postorder return value optimizations#31699
JIT: remove assert from postorder return value optimizations#31699AndyAyersMS merged 1 commit intodotnet:masterfrom
Conversation
We were trying to assert that we weren't losing side effects when folding return value trees. But this seems to be more trouble than it is worth as the set of side effects can also change. Also re-enable the disabled tests, and make one of them Pri0. Fixes dotnet#31665.
|
cc @trylek @dotnet/jit-contrib |
|
Reverts #31674 |
|
@AndyAyersMS Can you please provide more details? What side effect flag was set on |
Odd, I thought I had added provided notes on this in the PR or in the issue, but don't see them. Prior to this change, we would form: With this change, we fold and so get rid of the possibility of overflow exceptions: So as we walk up the tree the parent nodes still have the X flag, even though we've cleared it out in the children, and that was what triggered the assert. |
|
Perhaps a better fix is to call |
|
Do we know if |
|
I can just update flags in the JTRUE case; we already know we have a constant, so no need to worry about GTF_CALL. I don't need to do this update for other trees. diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp
index 9528f473615..23bc843a1d8 100644
--- a/src/coreclr/src/jit/flowgraph.cpp
+++ b/src/coreclr/src/jit/flowgraph.cpp
@@ -22617,8 +22617,10 @@ Compiler::fgWalkResult Compiler::fgLateDevirtualization(GenTree** pTree, fgWalkD
JITDUMP(" ... found foldable jtrue at [%06u] in BB%02u\n", dspTreeID(tree), block->bbNum);
noway_assert((block->bbNext->countOfInEdges() > 0) && (block->bbJumpDest->countOfInEdges() > 0));
- // Had hoped to assert here that we are not losing any
- // side effects, but can't find a way to express it properly.
+ // We have a constant operand, and should have the all clear to optimize.
+ // Update side effects on the tree, assert there aren't any, and bash to nop.
+ comp->gtUpdateNodeSideEffects(tree);
+ assert((tree->gtFlags & GTF_SIDE_EFFECT) == 0);
tree->gtBashToNOP();
BasicBlock* bTaken = nullptr; |
|
Sounds good to me. |
We were trying to assert that we weren't losing side effects when folding
return value trees. But this seems to be more trouble than it is worth as
the set of side effects can also change.
Also re-enable the disabled tests, and make one of them Pri0.
Fixes #31665.