On Mon, 25 Nov 2024 08:56:31 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
> I heard no argument about why you did not split this up. Please do that in > the future. It is hard to review well when there is this much code. If it is > really necessary, then sure. Here it does not seem necessary to deliver all > at once. > > > The patch includes IR framework-based scalar constant folding test points. > > You mention this IR test: > > https://github.com/openjdk/jdk/pull/21490/files#diff-3f8786f9f62662eda4b4a5c76c01fa04534c94d870d496501bfc20434ad45579R169-R174 > > Here I only see the use of very trivial values. I think we need more > complicated cases. > > What about these: > > * Add/Sub/Mul/Div/Min/Max ... with NaN and infinity. > * Same where it would overflow the FP16 range. > * Negative zero tests. > * Division by powers of 2. > > It would for example be nice if you could iterate over all inputs. FP16 with > 2 inputs is only 32bits, that can be iterated in just a few seconds. Then you > can run the computation with constants in the interpreter, and compare to the > results in compiled code. [ScalarFloat16OperationsTest.java](https://github.com/openjdk/jdk/pull/21490/files#diff-6afb7e66ce0fcdac61df60af0231010b20cf16489ec7e4d5b0b41852db8796a0) Adds has a specialized data provider that generates test vectors with special values, our functional validation is covering the entire Float16 value range. > src/hotspot/share/opto/divnode.cpp line 789: > >> 787: >> 788: if(t1 == TypeH::ZERO && !g_isnan(t2->getf()) && t2->getf() != 0.0) >> 789: return TypeH::ZERO; > > brackets for if > > Ok, why not also do it for negative zero then? Same as above, IEEE 754 spec treats both +ve and -ve zeros equally during comparison operations. jshell> 0.0f != 0.0f $1 ==> false jshell> 0.0f != -0.0f $2 ==> false jshell> -0.0f != -0.0f $3 ==> false jshell> -0.0f != 0.0f $4 ==> false > src/hotspot/share/opto/divnode.cpp line 797: > >> 795: >> //------------------------------isA_Copy--------------------------------------- >> 796: // Dividing by self is 1. >> 797: // If the divisor is 1, we are an identity on the dividend. > > Suggestion: > > // If the divisor is 1, we are an identity on the dividend. > > `Dividing by self is 1.` That does not seem to apply here. Maybe you meant > `dividing by 1 is self`? The comment mentions the divisor being 1. Looks fine. > src/hotspot/share/opto/divnode.cpp line 836: > >> 834: >> 835: // return multiplication by the reciprocal >> 836: return (new MulHFNode(in(1), >> phase->makecon(TypeH::make(reciprocal)))); > > Do we have good tests for this optimization? I have added a test point https://github.com/openjdk/jdk/pull/21490/files#diff-3f8786f9f62662eda4b4a5c76c01fa04534c94d870d496501bfc20434ad45579R203 I also added detailed comments to explain this better. > src/hotspot/share/opto/mulnode.cpp line 561: > >> 559: const Type *MulHFNode::mul_ring(const Type *t0, const Type *t1) const { >> 560: if( t0 == Type::HALF_FLOAT || t1 == Type::HALF_FLOAT ) return >> Type::HALF_FLOAT; >> 561: return TypeH::make( t0->getf() * t1->getf() ); > > I hope that `TypeH::make` handles the overflow cases well... does it? > And do we have tests for this? Please refer to following lines of code. https://github.com/openjdk/jdk/pull/21490/files#diff-3559dcf23b719805be5fd06fd5c1851dbd8f53e47afe6d99cba13a3de0ebc6b2R1446 There are two versions of TypeH::make, one with short and the other accepting floating point parameter, in the latter version we explicitly invoke a runtime help to convert float to float16 value, this shall take care of overflow scenario where we return an infinite Float16 value. There is no underflow in the case of a floating point number, for graceful degradation we enter into a sub-normal range and eventually return a zero value. On the other end of the spectrum i.e -ve values range we return a NEGATIVE_INFINITE, existing runtime helper is fully equipped to handle these cases. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21490#issuecomment-2498908764 PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857267174 PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266958 PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266304 PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266117
