Updating jit/valuenum to properly handle the single-precision versions of the math intrinsics. by tannergooding · Pull Request #9363 · dotnet/coreclr

tannergooding · 2017-02-06T06:48:41Z

This resolves https://github.com/dotnet/coreclr/issues/7689

tannergooding · 2017-02-06T06:51:48Z

tannergooding · 2017-02-06T06:55:12Z

 {
    assert(IsVNConstant(argVN));
    var_types argVNtyp = TypeOfVN(argVN);
+    assert(varTypeIsFloating(argVNtyp));


ValueNumStore::GetConstantDouble currently returns for both float and double constants. This means in some cases we are upcasting, executing some code, and then downcasting back to float.

After the reworking of EvalMathFuncUnary there are still a couple of places where this is happening (a simple search for GetConstantDouble in this file will show them). These calls could probably be rewritten to no longer up/down cast, but I would like some input before I do that work.

Yeah... in fact, it looks like we're already (without this change) constant-folding the single-precision intrinsics, just doing it by widening to double and evaluating and truncating back down, is that right? The language in the spec certainly seems to give us the liberty to do that (I.12.1.3). I'm inclined to say that we should either go ahead and make this change for all constant-folding on fp ops or not make this change, to keep things consistent.

I'm not sure whether it's best to continue folding at double precision or to switch to single. I'd think we'd want to minimize differences between optimized and unoptimized code's results, and I think the unoptimized code would end up happening at whatever precision our back-end uses for singles, which maybe has a greater chance of matching the C++ compiler's handling of float than of double, which would argue for continuing this change... but I'm no floating-point expert. Maybe someone else wants to chime in. @sivarv? @CarolEidt?

Yeah... in fact, it looks like we're already (without this change) constant-folding the single-precision intrinsics, just doing it by widening to double and evaluating and truncating back down, is that right?

Yes, which can result in lower performance and different computed results for the new System.MathF functions.

The language in the spec certainly seems to give us the liberty to do that (I.12.1.3).

I believe that most consumers of the single-precision functions would rather have the speed than the additional precision (including any additional precision retained by upcasting, computing, and downcasting).

I'd think we'd want to minimize differences between optimized and unoptimized code's results

I agree, and my understanding is that 'internal representation' is a platform agnostic way of saying whatever form the CPU accepts (so 80-bits when using the x87 FPU, for example). I think that using the C type names (float and double), calling the appropriate versions of the functions (sqrtf and sqrt, respectively), and then letting the native runtime handle the internal implementation details is the best way to go here.

@tannergooding, your reasoning sounds good to me; I'd say go ahead with this and the other instances.

JosephTremoulet

@dotnet/jit-contrib

JosephTremoulet · 2017-02-06T16:05:13Z

+
+    float result = 0;
+
+    switch (argVNtyp)


argVNTyp is only used in an assert and in a single-case switch with an unreached default case, which effectively asserts the same thing again. I'd suggest getting rid of argVNTyp (use TypeOfVN(argVN) in the assertion) and simplifying away the switch.

JosephTremoulet · 2017-02-06T16:08:24Z

+            assert(typ == TypeOfVN(arg0VN));
+
+            // If the math intrinsic is not implemented by target-specific instructions, such as implemented
+            // by user calls, then don't do constant folding on it. This minimizes precision loss.


ISTM this comment was explaining why && Compiler::IsTargetIntrinsic(gtMathFN) was included in the now-outer conditional; please move it back out.

JosephTremoulet · 2017-02-06T16:08:55Z

+            assert(typ == TypeOfVN(arg0VN));
+
+            // If the math intrinsic is not implemented by target-specific instructions, such as implemented
+            // by user calls, then don't do constant folding on it. This minimizes precision loss.


Ditto re: comment placement

JosephTremoulet · 2017-02-06T16:11:07Z


-            return VNForIntCon(int(res));
+            int res = 0;
+            if (gtMathFN == CORINFO_INTRINSIC_Round)


This if and the unreached() in its else branch are redundant with the assertion 3 lines above. I'd probably let the assertion do the talking (maybe add a comment that Round is the only intrinsic with fp args and a non-fp result) and get rid of the if/else.

JosephTremoulet · 2017-02-06T16:31:09Z

 {
    assert(IsVNConstant(argVN));
    var_types argVNtyp = TypeOfVN(argVN);
+    assert(varTypeIsFloating(argVNtyp));


Yeah... in fact, it looks like we're already (without this change) constant-folding the single-precision intrinsics, just doing it by widening to double and evaluating and truncating back down, is that right? The language in the spec certainly seems to give us the liberty to do that (I.12.1.3). I'm inclined to say that we should either go ahead and make this change for all constant-folding on fp ops or not make this change, to keep things consistent.

I'm not sure whether it's best to continue folding at double precision or to switch to single. I'd think we'd want to minimize differences between optimized and unoptimized code's results, and I think the unoptimized code would end up happening at whatever precision our back-end uses for singles, which maybe has a greater chance of matching the C++ compiler's handling of float than of double, which would argue for continuing this change... but I'm no floating-point expert. Maybe someone else wants to chime in. @sivarv? @CarolEidt?

…s of the math intrinsics.

tannergooding · 2017-02-22T15:28:04Z

Updated to handle all but one case where we are upcasting float to double when performing evaluations.

The remaining case is in Compiler::fgValueNumberTreeConst where it downcasts:

tree->gtVNPair.SetBoth(vnStore->VNForFloatCon((float)tree->gtDblCon.gtDconVal));

I'm taking a closer look at the code in question there to see if that is feasible to include here as well.

…rming evaluations.

tannergooding · 2017-02-22T23:38:19Z

@JosephTremoulet, this should be good for a final review pass now.

JosephTremoulet

LGTM, thanks for taking care of this.

tannergooding · 2017-02-23T01:33:00Z

@JosephTremoulet, thanks. Do I need any further sign-off here before merging?

JosephTremoulet · 2017-02-23T01:35:59Z

Do I need any further sign-off here before merging?

No, go ahead.

…sics Updating jit/valuenum to properly handle the single-precision versions of the math intrinsics. Commit migrated from dotnet/coreclr@116752f

dnfclas added the cla-already-signed label Feb 6, 2017

tannergooding commented Feb 6, 2017

View reviewed changes

JosephTremoulet reviewed Feb 6, 2017

View reviewed changes

tannergooding added 2 commits February 22, 2017 06:03

Updating jit/valuenum to properly handle the single-precision version…

43ea736

…s of the math intrinsics.

Dropping some unnecessary conditional statements in jit\valuenum.cpp

9f69d1f

Updating ValueNumStore to no longer upcast float to double when perfo…

8f05f42

…rming evaluations.

JosephTremoulet approved these changes Feb 23, 2017

View reviewed changes

tannergooding merged commit 116752f into dotnet:master Feb 23, 2017

karelz modified the milestone: 2.0.0 Aug 28, 2017

tannergooding deleted the math-intrinsics branch January 17, 2018 01:54

Conversation

tannergooding commented Feb 6, 2017

Uh oh!

tannergooding commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephTremoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Feb 22, 2017

Uh oh!

tannergooding commented Feb 22, 2017

Uh oh!

JosephTremoulet left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Feb 23, 2017

Uh oh!

JosephTremoulet commented Feb 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants