Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat negated float comparisons more directly #1487

Merged
merged 3 commits into from Feb 28, 2018

Conversation

Projects
None yet
6 participants
@lpw25
Copy link
Contributor

lpw25 commented Nov 21, 2017

This PR fixes the same problem as #1470 but rather than remove the optimisation of negated float comparisons it fixes it, and tries to ensure that similar bugs are not introduced again. The main part of the patch is replacing the comparison type:

and comparison =
    Ceq | Cne | Clt | Cgt | Cle | Cge

with two separate comparison types:

and integer_comparison =
    Ceq | Cne | Clt | Cgt | Cle | Cge

and basic_float_comparison =
    CFeq | CFlt | CFgt | CFle | CFge

and float_comparison =
  { basic : basic_float_comparison;
    negated : bool; }

with the float_comparison type now supporting all the negated comparisons as well.

This change is made to both Lambda and Cmm. These types are also introduced to Mach, but in that case it is merely a refactoring of what was already there since Mach could already represent the negated float comparisons.

An obvious benefit of this change is that it replaces the negation functions on comparisons:

val negate_comparison : comparison -> comparison

which were incorrect for float comparisons, with two correct functions:

val negate_integer_comparison : integer_comparison -> integer_comparison

val negate_float_comparison : float_comparison -> float_comparison
@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Nov 21, 2017

I think it's a good idea. Instead of a record type, I would have used a flat, enumerated type for FP comparisons:

  CFeq | CFneq | CFlt | CFnlt | CFgt | CFngt | CFle | CFnle | CFge | CFnge

just to make pattern-matchings syntactically lighter. It's not a big deal, though.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Nov 21, 2017

I tried that first, but it seemed to add more mess to a few backends than it took away from others. Happy to change it back if people prefer.

@lpw25 lpw25 force-pushed the lpw25:fix-float-compare-trunk branch from c9c22fe to 5d128d4 Nov 22, 2017

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Dec 6, 2017

@xavierleroy @lpw25 Shall we make a decision...?

| Lambda.Clt -> Clt
let transl_int_comparison cmp = cmp

let transl_float_comparison cmp = cmp

This comment has been minimized.

@murmour

murmour Dec 14, 2017

Contributor

These two functions are dummies now. Why not remove them, refactoring their uses?

This comment has been minimized.

@alainfrisch

alainfrisch Feb 28, 2018

Contributor

Given that the types are reexported in Cmm, they are somehow "decorrelated" from Lamdba, and those functions would make it easier to make them evolve independently. I think it is fine to keep those currently-identity functions.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 28, 2017

I had a second look and am still not completely convinced by the record type. Idioms such as

           match (cmp.basic, cmp.negated) with
           | (CFeq, false) -> "eq"

instead of

           match cmp with
           | { basic = CFeq; negated = false } -> "eq"

suggest that records are syntactically too heavy for this particular use. Alslo, the field name basic is not particularly evocative.

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Dec 28, 2017

Fair enough. I'll revert it back to using a variant type when I'm back from vacation.

@lpw25 lpw25 force-pushed the lpw25:fix-float-compare-trunk branch from 5d128d4 to 558d822 Jan 3, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

lpw25 commented Jan 3, 2018

Ok, it uses a flat variant type now.

@alainfrisch
Copy link
Contributor

alainfrisch left a comment

LGTM. And Xavier's comment was addressed.

@alainfrisch alainfrisch merged commit 1671e5a into ocaml:trunk Feb 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 1, 2018

trunk is currently broken, at least on zsystems and the breakage may have to
do with this PR.

Here is a list of failing tests with a bit of information:

tests/float-unboxing: Fatal error: exception Failure("gpr 1O9; alloc = 16")

tests/misc-unsafe/'almabench.ml' with 1 (native): the output values do not
match

tests/basic/'equality.ml' with 1 (native):
failure of tests 53, 54 and 55.

tests/misc/'nucleic.ml' with 1 (native):
the program output differs from the expected one:
-33.7976
+29.6107

tests/basic/'constprop.ml' with 2 (native):
also a mismatch in program output:
-floats: passed
+floats: FAILED

alainfrisch added a commit that referenced this pull request Mar 1, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Mar 1, 2018

Thanks @shindere ! I've pushed a tentative fix, directly to master. If this doesn't fix the problem, I suggest to revert.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.