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

Error with SML/NJ-built mlton: SML/NJ does not support EXACT formatting of reals #523

Closed
shwestrick opened this issue Nov 5, 2023 · 5 comments · Fixed by #525
Closed

Comments

@shwestrick
Copy link

shwestrick commented Nov 5, 2023

I have been experimenting with building MLton from scratch using SML/NJ. As of #520 (which fixed #519), the command make smlnj-mlton now succeeds, producing build/bin/mlton.smlnj.

Unfortunately, however, mlton.smlnj crashes due to a problem in SML/NJ's implementation of Real.fmt, which raises an exception for EXACT formatting (link).

For example:

$ cat test.sml
print (Int.toString (Real.round Math.pi));

$ build/bin/mlton.smlnj -verbose 2 test.sml
MLton 20231101.194947-g537750af9 starting
   Compile SML starting
      ... omitted ...
      ssaSimplify starting
         ... omitted ...
         constantPropagation starting
            constantPropagation raised: Fail: RealFormat: fmtReal: EXACT not supported
               Basis/Implementation/real-format.sml:264.10-264.57
         constantPropagation raised in 0.02 + 0.01 (33% GC)
      ssaSimplify raised in 0.15 + 0.06 (28% GC)
   Compile SML raised in 8.75 + 9.94 (53% GC)
MLton 20231101.194947-g537750af9 raised in 8.75 + 9.94 (53% GC)

And, one consequence of this is that make bootstrap-smlnj fails!

Some digging

I traced the crash back to RealX.toString here:

fun toString (r, {suffix}) =
let
val doit =
if suffix
then fn (r, s) => r ^ s
else fn (r, _) => r
in
case r of
Real32 r => doit (Real32.format (r, Real32.Format.exact), ":r32")
| Real64 r => doit (Real64.format (r, Real64.Format.exact), ":r64")
end

Which in turn triggers a call to Real.fmt StringCvt.EXACT, here:

fun format (x, f) = Real.fmt f x

Workaround

Here's a hack which appears to work:

diff --git a/lib/mlton/basic/real.sml b/lib/mlton/basic/real.sml
index 046a3c8a9..d9d1e0481 100644
--- a/lib/mlton/basic/real.sml
+++ b/lib/mlton/basic/real.sml
@@ -58,7 +58,10 @@ structure Format =
       val gen = GEN
    end
 
-fun format (x, f) = Real.fmt f x
+fun format (x, f) =
+  case f of
+    StringCvt.EXACT => Real.fmt (StringCvt.FIX (SOME 30)) x
+  | _ => Real.fmt f x

Discussion

@MatthewFluet curious to get your thoughts on this. Of course, it would be great if SML/NJ didn't have this problem, but alas, it does. And, it would be nice if bootstrapping with SML/NJ worked.

My understanding is that, for any code that contains constants of type real, these constants will need to be laid out as a string at some point (either internally for one of the IRs, or physically in the final output code).

Using a fixed precision of 30 in the workaround above seems like it would be "good enough". MLKit uses the same hack at the moment (link).

I wonder if it's possible to use this workaround only for mlton.smlnj, and continue to use EXACT formatting for normal mlton? If so, would you like me to try to implement that?

@shwestrick
Copy link
Author

shwestrick commented Nov 6, 2023

By the way, I've confirmed that, with this change, I'm able to build mlton from source using make bootstrap-smlnj on Ubuntu 22.04 🚀

@MatthewFluet
Copy link
Member

MatthewFluet commented Nov 6, 2023

Something must be wrong with the way the SML/NJ stubs are working, because that's supposed to be handled:

The Fail exception is coming straight from SML/NJ, so somehow missing the patching.

@MatthewFluet
Copy link
Member

I also don't know why ConstantPropagation is triggering the conversion of a real constant to a string, but, as you note, even if compilation got through the middle-end, the codegen will need to emit the real constant as a string.

@shwestrick
Copy link
Author

Something must be wrong with the way the SML/NJ stubs are working

Gotcha, this makes sense. I figured this might be something that the stubs were (supposed to be) handling... but not sure what's going wrong. I'll take a closer look later, or let me know what you find if you get to it first!

MatthewFluet added a commit to MatthewFluet/mlton that referenced this issue Nov 7, 2023
Closes MLton#523

Although SML/NJ 110.99.3 fixed some issues with `structure Real{,64}`
failing to match the SML Basis Library specification, it did not
implement `Real.fmt StringCvt.EXACT`.  So, a "fixed" version of
`structure Real64` needs to be exported (rather than exposing the
version of `structure Real64` from `$/basis.cm`) and it is simplest to
revert back to `structure Real64 = Real`
(where `structure Real = FixReal(struct open Pervasive.Real end)`).
@MatthewFluet
Copy link
Member

I also don't know why ConstantPropagation is triggering the conversion of a real constant to a string, but, as you note, even if compilation got through the middle-end, the codegen will need to emit the real constant as a string.

ConstantPropagation performs globalization of small values, using a hash table. Hashing of real constants is performed by converting the real value to a string and hashing the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants