Keep reads within buffer in AnalysisUpdateNeon().

webrtc/modules/audio_processing/ns/nsx_core_neon.c

Index: webrtc/modules/audio_processing/ns/nsx_core_neon.c
diff --git a/webrtc/modules/audio_processing/ns/nsx_core_neon.c b/webrtc/modules/audio_processing/ns/nsx_core_neon.c
index 65788ae23011f1a2f1bee4a28cebbd5500c96518..516dd095ccbc4c78af6b0ed892246aed215f6566 100644
--- a/webrtc/modules/audio_processing/ns/nsx_core_neon.c
+++ b/webrtc/modules/audio_processing/ns/nsx_core_neon.c
@@ -570,8 +570,8 @@ void WebRtcNsx_AnalysisUpdateNeon(NoiseSuppressionFixedC* inst,
   // Window data before FFT.
   int16_t* p_start_window = (int16_t*) inst->window;
   int16_t* p_start_buffer = inst->analysisBuffer;
+  int16_t* p_end_buffer = inst->analysisBuffer + inst->anaLen;

[peah-webrtc, 2016/04/01 05:26:32]

Could you please explain why this code changed. As far as I can see, the change
is bitexact as both p_start_out and p_start_buffer are updated in the same
manner and differ from p_end_out and p_end_buffer in the same manner.

However, since data is written into p_start_out and only read from
p_start_buffer, to me the former loop condition more clearly shows/promises that
there is no data written outside of the out buffer.

[Simon Hosie, 2016/04/01 21:57:46]

FTR; the change from testing the progress of the destination pointer to testing
the progress of the source pointer is because the source pointer leads the
destination pointer by one iteration, and so it will reach its limit first.

   int16_t* p_start_out = out;
-  const int16_t* p_end_out = out + inst->anaLen;
 
   // Load the first element to reduce pipeline bubble.
   int16x8_t window = vld1q_s16(p_start_window);
@@ -579,7 +579,7 @@ void WebRtcNsx_AnalysisUpdateNeon(NoiseSuppressionFixedC* inst,
   p_start_window += 8;
   p_start_buffer += 8;

[Simon Hosie, 2016/04/01 21:57:46]

Here is where the source pointer starts to lead the destination.

 
-  while (p_start_out < p_end_out) {
+  while (p_start_buffer < p_end_buffer) {
     // Unroll loop.
     int32x4_t tmp32_low = vmull_s16(vget_low_s16(window), vget_low_s16(buffer));
     int32x4_t tmp32_high = vmull_s16(vget_high_s16(window),
@@ -595,4 +595,11 @@ void WebRtcNsx_AnalysisUpdateNeon(NoiseSuppressionFixedC* inst,
     p_start_window += 8;
     p_start_out += 8;
   }
+  int32x4_t tmp32_low = vmull_s16(vget_low_s16(window), vget_low_s16(buffer));

[hlundin-webrtc, 2016/03/31 07:55:09]

Can you explain to me what this new code after the loop does?

[Simon Hosie, 2016/03/31 21:09:42]

The change to the loop condition is to make it run one iteration fewer.  We
still want to do all of that last iteration of work _except_ the `vld1q_s16()`
operations (which are preparation for the subsequent iteration); so this extra
code repeats the loop body with those removed.

+  int32x4_t tmp32_high = vmull_s16(vget_high_s16(window),
+                                   vget_high_s16(buffer));
+
+  int16x4_t result_low = vrshrn_n_s32(tmp32_low, 14);
+  int16x4_t result_high = vrshrn_n_s32(tmp32_high, 14);
+  vst1q_s16(p_start_out, vcombine_s16(result_low, result_high));

[peah-webrtc, 2016/04/01 05:26:32]

As far as I can see, p_start_out will be > out + inst->anaLen here, right? Is
that intended and safe?

As far as I can see, this differs from how it is done in the C counterpart
function AnalysisUpdateC() in nsx_core.c. 

Could you please explain this codechange further.

[Simon Hosie, 2016/04/01 06:12:59]

Changing the loop condition to work with the input buffer reduces the number of
iterations by one (because `p_start_buffer` has already been incremented once
before the loop), so the writes should stay within range in both cases.  Before
the change the final iteration would perform reads for the subsequent iteration,
which is never used so it's likely that those reads are unsafe (I haven't
actually confirmed the the safety, but it's bad practice and should be corrected
anyway).

 }