Prerequisite work item for the CSE of GT_CNS_INT for ARM64 work item - #39021
Conversation
… diffs in the framework libraries) Mark nodes that use the division by constant optimization with GTF_DIV_BY_CNS_OPT Don't perform const prop on expressions marked with GTF_DONT_CSE, as this would undo a constant CSE Fix for bug in AssertionProp where we assign the wrong value number when folding a conditional When dumping the BasicBlocks print hascall when the block is marked with BBF_HAS_CALL Call CheckDivideByConstOptimized when early prop inserts a constant node added methods: UsesDivideByConstOptimized, CheckDivideByConstOptimized and MarkDivideByConstant Propagate any side effect flags in the gtCallAddr field of an indirect call node Call CheckDivideByConstOptimized when morphing a divide or remainder nodes Don't allow changing a floating point GT_DIV into a GT_MUL in fgMorph after the global morph phase In loop hoisting, set BBF_HAS_CALL if we hoist a tree that contains a call When hoisting something that requires a physical register, clear that requirement in the hoisted copy
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
@sandreenko PTAL |
sandreenko
left a comment
sandreenko
left a comment
There was a problem hiding this comment.
LGTM, 2 questions and a few nits.
Thank you for extracting this.
| //------------------------------------------------------------------------ | ||
| // CheckDivideByConstOptimized: | ||
| // Checks if we will can use the division by constant optimization | ||
| // on this node |
There was a problem hiding this comment.
// on this node
// and if so sets the flag GTF_DIV_BY_CNS_OPT and
it looks like an unintentional new line.
| GenTree* divisor = gtGetOp2()->gtEffectiveVal(/*commaOnly*/ true); | ||
| if (divisor->OperIs(GT_CNS_INT)) | ||
| { | ||
| divisor->gtFlags |= GTF_DONT_CSE; |
There was a problem hiding this comment.
you have answered earlier why we check GTF_DONT_CSE in optVNConstantPropOnJTrue and as I understood the idea was to replace constant values with CSE lclVars, is my understanding correct?
If so why do we forbid replacing these const with a CSE lclVar here?
There was a problem hiding this comment.
Because changing the CNS_INT to a CSE LclVar will cause lower to fail to use the multiplication by reciprocal optimization.
Thus we would generate a slow divide instruction instead of the faster multiply or shift sequence.
There was a problem hiding this comment.
Ah, I think I understand now.
From these two comments:
Doing a constant prop here would replace the CSE LclVar with the original constant.
Essentially undoing the CSE of the constant.
Because changing the CNS_INT to a CSE LclVar will cause lower to fail to use the multiplication by reciprocal optimization.
In general we want to set DONT_CSE on constants under DIV/MOD so they are not replaced with a CSE LCL_VAR. Other constants (that are not under 'DIV/MOD) are not marked as DONT_CSEso they could be replaced and, once they are replaced, we mark it withDONT_CSE` because it is their final state. Is it correct?
| } | ||
| else | ||
| { | ||
| printf(" -0x%llx", -dspIconVal); |
There was a problem hiding this comment.
Starting from VS2013 we have support for "%zd" (and other runtime parts are already using it), so I would suggest to just replace this block with:
printf(" %zd", dspIconVal);
that will handle both 32/64 and negative/positive.
Note: gcc and clang have always supported that.
There was a problem hiding this comment.
I like see these numbers in hex, especially with my shared CSE constant changes where we often have to add or subtract a small offset.
There was a problem hiding this comment.
I have not seen hex numbers being printed with a negative sign, but maybe it is fine. Thanks for the explanation.
| // this - a GenTreeOp node | ||
| // comp - the compiler instance | ||
| // | ||
| void GenTreeOp::CheckDivideByConstOptimized(Compiler* comp) |
There was a problem hiding this comment.
do we want this to happen in minopts (opts.OptimizationEnabled() == false?
There was a problem hiding this comment.
I will add a check for minopts to UsesDivideByConstOptimization()
…on for UDIV and UMOD with a power of two divisor.
ghost
locked as resolved and limited conversation to collaborators
3 participants
This change has zero code diffs in the framework libraries