JIT: Handle GT_CNS_LNG and GT_CNS_DBL in GenTree::Compare #102137
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib |
|
|
||
| case GT_CNS_DBL: | ||
| { | ||
| if (op1->AsDblCon()->isBitwiseEqual(op2->AsDblCon())) |
There was a problem hiding this comment.
What’s the intent of GenTree::Compare here?
I feel like having an explanatory comment around why this type of comparison is fine for double is really necessary to elaborate on why NaN == NaN is okay for bitwise equivalent.
Such a comment would also help clarify if this could be updated to be more or less inclusive based on IEEE 754/ECMA 335 rules and whether or not this needs to ensure that float vs double typing for CNS_DBL needs to be considered since we only have the one node
There was a problem hiding this comment.
GenTree::Compare returns true if two trees are identical. This comment already exists on the function itself.
There was a problem hiding this comment.
There seems to be a lot of potentially dangerous nuance here.
Just looking at many of the callers, many (but not all) are having to explicitly skip handling of floating-point or NaNs/etc themselves when considering GenTree::Compare. It seems like one of those cases where it might be applicable to simply say that two floating-point nodes containing NaN are not equal ever and otherwise only if bitwise equal; just to help avoid potential problems.
There was a problem hiding this comment.
gtFoldExprCompare for example uses this to see "if two trees are identical, then folding is possible"
but of course it has to manually account for floating-point and skip it today, when in practice it could do it for everything except NaN (although even NaN can be folded in many cases in practice since we know such comparisons always produce false)
There was a problem hiding this comment.
GenTree::Compare is a syntactic equality check. It does not say anything about whether two trees compute to the same value under any form of comparison. This is a fundamental property of syntax vs semantics that callers need to take into account when they make use of it. For example, two syntactically identical calls for obvious reasons are not guaranteed to evaluate to the same value, even though GenTree::Compare returns true for them.
I do not see a good reason to pessimize GenTree::Compare for the callers that do use it correctly (like tail/head merging) to account for other callers potentially using it incorrectly. Those other callers should be fixed to realize that fundamentally GenTree::Compare cannot be used to guarantee in general that two trees produce the same value under any form of comparison.
No description provided.