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

Set default optimization level to -O1 #2411

Merged
merged 4 commits into from Aug 29, 2018

Conversation

galv
Copy link
Contributor

@galv galv commented May 10, 2018

Previously, we were building at -O0 by default.

Since gcc uses only the rightmost command line -O flag, override this
by setting the environment variable EXTRA_CXXFLAGS.

@galv
Copy link
Contributor Author

galv commented May 10, 2018

Don't merge this quite yet. There are a bunch of apparently new warnings due to inlining that will need to be dealt with.

@galv
Copy link
Contributor Author

galv commented May 11, 2018

We are getting lots of warning from -Wunused-result and -Wmaybe-uninitialized.

build.log

I'll see what I can do.

@galv
Copy link
Contributor Author

galv commented May 15, 2018

Many false positives about unused variables caused by KALDI_ERR << "my error message" not being interpreted by gcc as throwing an exception. I'll see what I can do about this.

i.e., in:

int x;
if (condition) {
  x = 2
} else {
  KALDI_ERR << "something wrong"
}

gcc cannot figure out that the else block throws an exception, so it thinks that x may be uninitialized after the if-else block. Maybein the destructor of the MessageLogger class is inlined, we are good, but I'm not sure what implications that has for the destructor not being no-except. Very tricky issue.

@galv
Copy link
Contributor Author

galv commented May 20, 2018

I'm not sure when I'll get around to fixing these warnings, unfortunately. If any C++ gurus want to take a stab at removing all warnings caused by -O1, say so, and I can make sure you have my latest code.

@danpovey
Copy link
Contributor

danpovey commented May 20, 2018 via email

@jtrmal
Copy link
Contributor

jtrmal commented May 20, 2018 via email

@jtrmal
Copy link
Contributor

jtrmal commented May 20, 2018 via email

@galv
Copy link
Contributor Author

galv commented May 20, 2018 via email

@danpovey
Copy link
Contributor

danpovey commented May 20, 2018 via email

@jtrmal
Copy link
Contributor

jtrmal commented Jun 13, 2018

@galv I'm trying to figure out something with travis -- could you push something into this branch?
Any change is fine, just looking if the checks will change

@jtrmal jtrmal closed this Jun 13, 2018
@jtrmal jtrmal reopened this Jun 13, 2018
@jtrmal
Copy link
Contributor

jtrmal commented Jun 13, 2018

@galv nevermind, I closing/reopening the PR restarted the checks (which is what I wanted)

@galv
Copy link
Contributor Author

galv commented Jun 13, 2018 via email

src/configure Outdated
@@ -42,7 +42,7 @@

# This should be incremented after any significant change to the configure
# script, i.e. any change affecting kaldi.mk or the build system as a whole.
CONFIGURE_VERSION=7
CONFIGURE_VERSION=8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@galv, the configure version is already 8 now (and I don't know why the diff isn't vs. the master). Anyway-- I think if you just bump it up to 9, we should be good to go with this. Let's not overthink it.

@galv
Copy link
Contributor Author

galv commented Aug 24, 2018

Right, I remember Justin encountered this. Let me take a look at this again today, after I grab a late lunch.

Previously, we were building at -O0 by default.

Since gcc uses only the rightmost command line -O flag, override this
by setting the environment variable EXTRA_CXXFLAGS.
@galv
Copy link
Contributor Author

galv commented Aug 25, 2018

We are getting way too many -Wmaybe-uninitialized-variable warnings because gcc does not realize that KALDI_ERR and KALDI_ASSERT do not return if we enable -O1. I need to either fix this As Yenda suggested, or maybe just add -Wno-maybe-uninitialized as a quick fix.

@danpovey
Copy link
Contributor

danpovey commented Aug 25, 2018 via email

These were caused by compiler not realizing that KALDI_ERR and
KALDI_ASSERT don't return. Solution came from suggestions by Yenda,
Dan Povey, and the GLOG library.

Still needs to be cleaned up a little bit.
It is possible that the two GetVerboseLogs() may return different
values, because the global verbosity varible could change.
@galv
Copy link
Contributor Author

galv commented Aug 27, 2018

I've figured something out. See my latest commits if you are curious. It's inspired by the google logging library, which faced the same problem. It's not totally polished yet. I'll see if I can clear time in my day tomorrow to clean this up and finish this of.

fl = si/(float)(4*256);
syn_d[aa] = fl;*/
}
}

saveWeights();

// idiom to "use" an unused variable
(void) unused_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might be able to achieve the same thing by declaration
volatile in unused;
but your solution might be better (could get optimized better), however at the cost that you have remember to "use" the unused variable in each path through the code.

@jtrmal
Copy link
Contributor

jtrmal commented Aug 27, 2018 via email

@danpovey
Copy link
Contributor

i found the reason for test failure--- was unrelated I think. will merge.

@danpovey danpovey merged commit aa75689 into kaldi-asr:master Aug 29, 2018
@galv
Copy link
Contributor Author

galv commented Aug 29, 2018

@danpovey Could I kindly ask you to revert this merge? I just finished removing all of the warnings generated at -O1 last night, but I did not push it because I had plans that night and wanted to look it over one last time. I apologize, but I may not have communicated to you clearly how many warnings that this change will produce when kaldi is compiled (feel free to check for yourself).

@galv
Copy link
Contributor Author

galv commented Aug 29, 2018

Check out my most recent push on my personal branch to see the extent to which I had to change files to silence extraneous warnings: galv@57a0bee

I am running tests now to make sure that I haven't broken anything, but the tests themselves also generate new warnings, so I will need to silence the warnings in those too.

I don't think this is ready yet.

@danpovey
Copy link
Contributor

danpovey commented Aug 29, 2018 via email

@galv
Copy link
Contributor Author

galv commented Aug 29, 2018

Okay, well, I guess it's an incentive for me to actually get this done quickly then.

@jtrmal
Copy link
Contributor

jtrmal commented Aug 31, 2018

I wonder if you could also add explicit -fno-fast-math with a comment that enabling fast-math leads to improper handling of NaN and Inf (in the IEEE754 sense) and kaldi will not function properly in that case?

@danpovey
Copy link
Contributor

danpovey commented Aug 31, 2018 via email

@jtrmal
Copy link
Contributor

jtrmal commented Aug 31, 2018 via email

danpovey added a commit that referenced this pull request Sep 3, 2018
dpriver pushed a commit to dpriver/kaldi that referenced this pull request Sep 13, 2018
Skaiste pushed a commit to Skaiste/idlak that referenced this pull request Sep 26, 2018
Skaiste pushed a commit to Skaiste/idlak that referenced this pull request Sep 26, 2018
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 this pull request may close these issues.

None yet

3 participants