Add missing logging to gtFoldExpr #47757
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
SingleAccretion
force-pushed
the
Improve-Logging-In-GtFoldExpr
branch
from
060d17c to
ea818cf
Compare
src/coreclr/jit/gentree.cpp
Outdated
| #ifdef DEBUG | ||
| if (verbose) | ||
| { | ||
| printf("\nIdentical GT_COLON trees! "); | ||
| gtDispTree(op2); | ||
| } | ||
| #endif | ||
| GenTree* op; | ||
| if (sideEffList == nullptr) | ||
| { | ||
| // No side-effects, just return colon_op2 | ||
| return colon_op2; | ||
| JITDUMP("No side effects, bashing to second operand:\n") | ||
| op = colon_op2; | ||
| } | ||
| else | ||
| { | ||
| #ifdef DEBUG | ||
| if (verbose) | ||
| { | ||
| printf("\nIdentical GT_COLON trees with side effects! Extracting side effects...\n"); | ||
| printf("Extracting side effects...\n"); | ||
| gtDispTree(sideEffList); | ||
| printf("\n"); | ||
| } | ||
| #endif | ||
| // Change the GT_COLON into a GT_COMMA node with the side-effects | ||
| op2->ChangeOper(GT_COMMA); | ||
| op2->gtFlags |= (sideEffList->gtFlags & GTF_ALL_EFFECT); | ||
| op2->AsOp()->gtOp1 = sideEffList; | ||
| return op2; | ||
|
|
||
| JITDUMP("Transformed GT_COLON into GT_COMMA:\n"); | ||
| op = op2; | ||
| } | ||
| #ifdef DEBUG | ||
| if (verbose) | ||
| { | ||
| gtDispTree(op); | ||
| } | ||
| #endif | ||
| return op; |
There was a problem hiding this comment.
It is notable that this part of the change is most involved and at the same time probably least useful. Quick survey of places where QMARK nodes are created suggests they are quite unlikely to end up with identical trees for the COLON.
Ominously, commenting out this optimization results in 0 diffs across the framework assemblies when crossgening...
SingleAccretion
force-pushed
the
Improve-Logging-In-GtFoldExpr
branch
3 times, most recently
from
cfc6efc to
c86df90
Compare
SingleAccretion
force-pushed
the
Improve-Logging-In-GtFoldExpr
branch
from
c86df90 to
3453ae2
Compare
|
cc @dotnet/jit-contrib |
There was a problem hiding this comment.
LGTM, nicely done!
I agree with your approach to follow the existing dumping logic in these functions.
I have verified this is a no-diff change across the framework assemblies when crossgening.
Great, have you already used jit-diffs before? Were there any issues with instructions/setup?
I did not add calls to DEBUG_DESTROY_NODE as it looks like the function is little used (only 65 hits across the entire codebase) and the printing for it is commented out... Please let me know if that's still desirable, but it would potentially result in a lot of churn if done properly for all folding functions.
We are trying to add them for new transformations but I am fine with keeping this code without them for now if it adds too much noise.
Yep (#47133)!
Fortunately no, the setup was painless and everything works as it should. |
I see. Since this a bit of a bigger work item, I will make a separate PR. |
ghost
locked as resolved and limited conversation to collaborators
Partially resolves #43348.
The first commit covers the logging for all paths in
gtFoldExpr. OthergtFold*functions seem to already have the necessary logging so I did not touch them. I followed the existing (ingtFoldExprConst) "fine-grained" approach instead of e. g. checking at the end ofgtFoldExprif the node has changed.I did not add calls to
DEBUG_DESTROY_NODEas it looks like the function is little used (only 65 hits across the entire codebase) and the printing for it is commented out... Please let me know if that's still desirable, but it would potentially result in a lot of churn if done properly for all folding functions.Edit: will add
DEBUG_DESTROY_NODEs in a separate PR.One thing I've noticed is that it would potentially be useful to have something like
gtDebugDisp*wrappers so that all non-trivial calls wouldn't have to be wrapped in#ifdef DEBUG+if (verbose), which adds quite a bit of noise. Please let me know if that's something desirable (potentially as a future PR) or the existing approach is fine as is.I have verified this is a no-diff change across the framework assemblies when crossgening.
cc @sandreenko