Repository navigation
Refactor debug-only GenTree flags. - #6002
Conversation
Move the four debug-only GenTree flags into their own field. This frees up two bits across both the RyuJIT and the legacy backend.
|
@dotnet/jit-contrib @mikedn PTAL This is part of #5999. |
| #define GTF_NODE_SMALL 0x00020000 | ||
|
|
||
| // Property of the node itself, not the gtOper | ||
| #define GTF_NODE_MASK (GTF_COLON_COND | GTF_MORPHED | GTF_NODE_SMALL | GTF_NODE_LARGE ) |
There was a problem hiding this comment.
Something bugs me about this. GTF_NODE_MASK is used to prevent copying these flags to other nodes but it contains only 3 of the 4 moved flags. I suspect that some places that deal with flag copying might need to be updated. For example, at the end of gtClone there is:
copy->gtFlags |= tree->gtFlags & ~GTF_NODE_MASK;
Shouldn't the following line be added
copy->gtDebugFlags |= tree->gtDebugFlags & ~(GTF_DEBUG_MORPHED | GTF_DEBUG_NODE_SMALL | GTF_DEBUG_NODE_LARGE);
to copy GTF_DEBUG_VAR_CSE_REF?
Though it's not clear why GTFD_VAR_CSE_REF would be copied in this case, maybe the existing code is wrong to begin with.
There was a problem hiding this comment.
Yes, I think you're correct. I'll run through the uses of GTF_NODE_MASK.
There was a problem hiding this comment.
I believe that we should get rid of the GTF_MORPHED flag as it isn't really very useful or even consistently set. It mostly hit us when you try to morph a node a second time in the global morph phase and that is something that we often need to do.
This is an orthononal issue, so feel free to commit this change without addressing this.
There was a problem hiding this comment.
I agree with @briansull about the GTF_MORPHED flag - I supposed it prevents some small amount of unintended throughput loss from accidentally re-morphing nodes, but is a huge hassle if that's what you really want to do.
I also agree that it is orthogonal to this.
There was a problem hiding this comment.
|
I'm not sure about the |
This also renames `GTF_DEBUG_MORPHED` to `GTF_DEBUG_NODE_MORPHED` for consitency with the other node-specific debug flags.
| // Print the tree so we can see it in the log. | ||
| printf("Missing flags on tree [%X]: ", tree); | ||
| GenTree::gtDispFlags(chkFlags & ~treeFlags); | ||
| GenTree::gtDispFlags(chkFlags & ~treeFlags, 0); |
There was a problem hiding this comment.
Just a suggestion - it might be nice to have a GTF_NONE and/or a GTF_DEBUG_NONE for cases like this, just to make it clearer from the caller's side what this is.
|
LGTM |
This is a marker that is used to indicate the absence of debug GenTree flags.
|
LGTM |
Refactor debug-only GenTree flags. Commit migrated from dotnet/coreclr@7081a2b
Move the four debug-only GenTree flags into their own field. This frees
up two bits across both the RyuJIT and the legacy backend.