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

Option to disable -Ofast #1433

Closed
FilippoC opened this issue Aug 23, 2018 · 1 comment
Closed

Option to disable -Ofast #1433

FilippoC opened this issue Aug 23, 2018 · 1 comment

Comments

@FilippoC
Copy link
Contributor

FilippoC commented Aug 23, 2018

Hello,

I am currently working on VAEs & parsing [1].
One requirement in this setting is to compute the entropy of the distribution over parse trees (i.e. for the KL divergence w.r.t. a flat prior).
To prevent computational errors (underflow, overflow, etc), we have to:

  • store signed values in log domain, see [2]
  • rely on careful implementation of operations like log(1+exp(x)), see [3]

However, the makefile of dynet forces the -Ofast option, which in turn force the -ffast-math option.
Therefore, we still encounter computational errors (e.g. exp(-92) results in an error).

To bypass this problem, I manually change the configuration and replace -Ofast with -O3 here:
https://github.com/clab/dynet/blob/master/CMakeLists.txt#L85

Would you accept a pull request that:

  • either replace -Ofast with -O3? (I think that Eigen use build in math operations, so it should not impact Dynet)
  • or add an option that controls CMake so the user can manually change it, e.g. -DGCC_O=3, -DGCC_O=fast, -DGCC_O=2, etc...

Note: To be clear, when I link my C++ program to Dynet, it changes the behavior of std::exp in my own code, which is annoying. It has nothing to do with any part of the code of Dynet, I am not talking about the dynet::exp operation.

[1] https://arxiv.org/abs/1807.09875
[2] http://www.cs.jhu.edu/~zfli/pubs/semiring_translation_zhifei_emnlp09.pdf
[3] https://cran.r-project.org/package=Rmpfr/vignettes/log1mexp-note.pdf

@neubig
Copy link
Contributor

neubig commented Aug 23, 2018

Hi, I think that this is a something worth considering. I've previously encountered problems related to this as well.

I think we'd need to profile to make sure it wasn't causing major performance issues though.

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

No branches or pull requests

2 participants