Fixed strict-aliasing issue using union in const ARPA related code; A… #211
Conversation
…lso fixed some style issue that I saw in the related code
| arpa_line.logprob = *reinterpret_cast<float*>(lm_state); | ||
| arpa_line.backoff_logprob = *reinterpret_cast<float*>(lm_state + 1); | ||
| Int32AndFloat logprob_i(*lm_state); | ||
| arpa_line.logprob = logprob_i.f; |
There was a problem hiding this comment.
I don't see why a temporary is needed... just use lm_state->f.
There was a problem hiding this comment.
lm_state has type of int32*. Or perhaps you mean casting lm_state to
Int32AndFloat using something like ((union Int32AndFloat *)lm_state)->f?
That may still cause strict aliasing problem?
Guoguo
On Mon, Oct 5, 2015 at 8:00 PM, Daniel Povey notifications@github.com
wrote:
In src/lm/const-arpa-lm.cc
#211 (comment):@@ -937,8 +944,10 @@ void ConstArpaLm::WriteArpaRecurse(int32* lm_state,
// Inserts the current LmState to .
ArpaLine arpa_line;
arpa_line.words = seq;
- arpa_line.logprob = reinterpret_cast<float>(lm_state);
- arpa_line.backoff_logprob = reinterpret_cast<float>(lm_state + 1);
- Int32AndFloat logprob_i(*lm_state);
- arpa_line.logprob = logprob_i.f;
I don't see why a temporary is needed... just use lm_state->f.
—
Reply to this email directly or view it on GitHub
https://github.com/kaldi-asr/kaldi/pull/211/files#r41213113.
There was a problem hiding this comment.
OK, I see. I had assumed that you had redefined lm_state to be that union.
Probably the compiler can optimize this fine..
did you see whether it affects speed? [I don't expect it will, but just
wonder, if it's easy to test.]
Dan
On Mon, Oct 5, 2015 at 8:11 PM, chenguoguo notifications@github.com wrote:
In src/lm/const-arpa-lm.cc
#211 (comment):@@ -937,8 +944,10 @@ void ConstArpaLm::WriteArpaRecurse(int32* lm_state,
// Inserts the current LmState to .
ArpaLine arpa_line;
arpa_line.words = seq;
- arpa_line.logprob = reinterpret_cast<float>(lm_state);
- arpa_line.backoff_logprob = reinterpret_cast<float>(lm_state + 1);
- Int32AndFloat logprob_i(*lm_state);
- arpa_line.logprob = logprob_i.f;
lm_state has type of int32_. Or perhaps you mean casting lm_state to
Int32AndFloat using something like ((union Int32AndFloat *)lm_state)->f?
That may still cause strict aliasing problem? Guoguo
… <#1503a7d7b26b61b2_>
On Mon, Oct 5, 2015 at 8:00 PM, Daniel Povey notifications@github.com
wrote: In src/lm/const-arpa-lm.cc <
https://github.com/kaldi-asr/kaldi/pull/211#discussion_r41213113>: > @@
-937,8 +944,10 @@ void ConstArpaLm::WriteArpaRecurse(int32_ lm_state, > //
Inserts the current LmState to . > ArpaLine arpa_line; >
arpa_line.words = seq; > - arpa_line.logprob =
reinterpret_cast<float>(lm_state); > - arpa_line.backoff_logprob =
reinterpret_cast<float>(lm_state + 1); > + Int32AndFloat
logprob_i(*lm_state); > + arpa_line.logprob = logprob_i.f; I don't see why
a temporary is needed... just use lm_state->f. — Reply to this email
directly or view it on GitHub <
https://github.com/kaldi-asr/kaldi/pull/211/files#r41213113>.—
Reply to this email directly or view it on GitHub
https://github.com/kaldi-asr/kaldi/pull/211/files#r41213815.
|
My reply somehow didn't get posted here, not sure if you received. "On WSJ it doesn't seem to have an impact on the speed." |
|
OK thanks, merging then. |
Fixed strict-aliasing issue using union in const ARPA related code; A…
…lso fixed some style issue that I saw in the related code. Tested this on WSJ and I was able to generate the same binary G.carpa as before.