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

Fix inequality for floating-point NaNs #11229

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Sep 18, 2021

IEEE 754 defines inequality through the compareQuietNotEqual predicate, which returns true if the two operands are unordered; x != y would be true if either operand is a NaN (to avoid confusion, earlier revisions of that standard gave it the ?<> symbol). Thus the correct LLVM predicate to use is UNE (unordered or not equal), not ONE (ordered and not equal). The docs for the #!= primitives are updated to reflect this.

See #11227.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric labels Sep 18, 2021
@oprypin
Copy link
Member

oprypin commented Sep 18, 2021

Hold up... These specs take a huge amount of time to run. Each run( call is a full-blown compiler invocation, and there are 252 of them here.


crystal spec/compiler/codegen/arithmetics_spec.cr

Finished in 5:10 minutes
399 examples, 0 failures, 0 errors, 0 pending

@oprypin
Copy link
Member

oprypin commented Sep 18, 2021

So I think the examples could use some merging. That will save a lot of run time. For starters, perhaps it would be better to unwrap the loop {% for op in %w(== != < <= > >=) %}.

The merging could just be about moving the branching point, i.e. still using macro loops, just within the run() somehow.

But additionally, I think that the conventional wisdom is to minimize control flow and indirections in unittests, so that any potential bugs in unittests can jump out immediately from looking at the code. So perhaps actually eliminating some of the loops would also be my style suggestion.

@HertzDevil
Copy link
Contributor Author

The majority of time is spent not on running the full compiler per se, but rather on including the entire prelude inside the other existing specs (because they need it for exception handling). Those 252 specs took only 38 seconds on my machine.

@straight-shoota
Copy link
Member

I think a good compromise could be to only implement a few essential compiler tests, in order to keep the number of individually compiled examples low.

The full amount of tests can be added as stdlib tests. We're actually testing a stdlib method, which just happens to be a primitive implemented in the compiler.

@HertzDevil
Copy link
Contributor Author

The same tests would break CI if they were tested in the standard library specs because, from what I understand, std_spec is built on some workflows before the second generation of the compiler with the codegen fix. So those tests would have to be excluded from such a run of std_spec (probably with a tag), and entirely omitted from workflows that don't rebuild the compiler. Is that still fine?

@HertzDevil
Copy link
Contributor Author

An alternative is to physically batch tests like so:

run(%(
  nan = 0.{{ conv.id }} / 0.{{ conv.id }}
  t1 = (nan == nan) ? 0b100000 : 0
  t2 = (nan != nan) ? 0b010000 : 0
  t3 = (nan <  nan) ? 0b001000 : 0
  t4 = (nan <= nan) ? 0b000100 : 0
  t5 = (nan >  nan) ? 0b000010 : 0
  t6 = (nan >= nan) ? 0b000001 : 0
  t1 &+ t2 &+ t3 &+ t4 &+ t5 &+ t6
  )).to_i.should eq(0b010000)

@oprypin
Copy link
Member

oprypin commented Sep 18, 2021

@HertzDevil I'm currently not sure regarding the exact merits but your example achieves exactly what I meant.

@straight-shoota
Copy link
Member

Yes, the stdlib specs would need to be moved behind a guard that makes sure the compiler contains the fix.
We could delay adding them until after the next release as a less complex solution.

Combining the results of multiple operations (or operand varieties) into a single run sounds good to me as well.
I believe the code should compile without prelude.

@asterite
Copy link
Member

I think the best thing to do is to test this in std specs, but under a macro if only for next compiler versions. Since CI runs std specs also with the new compiler,that should be enough.

@oprypin
Copy link
Member

oprypin commented Dec 4, 2021

Do I understand correctly that merging #11298 opened a path to still add all these specs but with much faster run time?
In that case this is probably ready for final reviews.

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 7, 2021
@straight-shoota straight-shoota merged commit 96b4209 into crystal-lang:master Dec 9, 2021
@HertzDevil HertzDevil deleted the bug/float-inequality branch December 10, 2021 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:codegen topic:stdlib:numeric
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants