[patch, libgfortran] PR53029 missed optimization in internal read (without implied-do-loop)

[patch, libgfortran] PR53029 missed optimization in internal read (without implied-do-loop)

[Jerry DeLisle]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

Hi all,

The problem here is that we never set the err return to LIBERROR_END in all 
cases. For the example case we are detecting the EOF condition inside the 
read_integer procedure and it gets acted on correctly at higher levels in the 
code. Consequently in the big loop over the array where we call 
list_formatted_read_scalar, we never returned an error code so we never exited 
the loop early.

The patch tests for the EOF first locally as before, but then returns the error 
flags set in dtp->common.flags which are set globally in the individual read 
procedures whene hit_eof is called.

Regression tested on x86_64. I have added a test case which will check the 
execution time of the loop. The previous results of the REAd were correct, just 
took a long time on large arrays.

OK for trunk?

Regards,

Jerry

2017-05-28  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libgfortran/35339
	* list_read.c.c (list_formatted_read_scala): Set the err return
	value to the common.flags error values.

[-- Attachment #2: pr35339.diff --]
[-- Type: text/x-patch, Size: 784 bytes --]

diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index 6c00d11b..b6cd6670 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -2298,11 +2298,16 @@ list_formatted_read_scalar (st_parameter_dt *dtp, bt type, void *p,
     free_saved (dtp);
 
 cleanup:
+  /* err may have been set above from finish_separator, so if it is set
+     trigger the hit_eof. The hit_eof will set bits in common.flags.  */
   if (err == LIBERROR_END)
     {
       free_line (dtp);
       hit_eof (dtp);
     }
+  /* Now we check common.flags for any errors that could have occurred in
+     a READ elsewhere such as in read_integer.  */
+  err = dtp->common.flags & IOPARM_LIBRETURN_MASK;
   fbuf_flush_list (dtp->u.p.current_unit, LIST_READING);
   return err;
 }

[-- Attachment #3: read_5.f90 --]
[-- Type: text/x-fortran, Size: 669 bytes --]

! { dg-do run }
! PR35339 Missed optimization, this test case took several seconds to
      program internalread
      implicit none
      integer m
      parameter(m=1000000)
      character value*10
      character(80) :: result
      integer i,j,intvalues(m)
      real :: start, finish
      intvalues = 33
      call cpu_time(start)
      do j=1,100
        write(value,'(i3,a5)') j," 5 69"
        read(value,*,end=20) intvalues
   20   write(result,*) (intvalues(i),i=2,4)
      if (result.ne.('           5          69          33')) call abort
      call cpu_time(finish)
      if ((finish-start).gt. 2.0) call abort
      enddo
      end program internalread

Re: [patch, libgfortran] PR53029 missed optimization in internal read (without implied-do-loop)

[Manfred Schwarb]

Am 29.05.2017 um 01:16 schrieb Jerry DeLisle:
> Hi all,
> 
> The problem here is that we never set the err return to LIBERROR_END in all cases. For the example case we are detecting the EOF condition inside the read_integer procedure and it gets acted on correctly at higher levels in the code. Consequently in the big loop over the array where we call list_formatted_read_scalar, we never returned an error code so we never exited the loop early.
> 
> The patch tests for the EOF first locally as before, but then returns the error flags set in dtp->common.flags which are set globally in the individual read procedures whene hit_eof is called.
> 
> Regression tested on x86_64. I have added a test case which will check the execution time of the loop. The previous results of the REAd were correct, just took a long time on large arrays.
> 

Seems to work as advertised.
With this small patch, I see a tremendous speedup for array reads.

The implied-do variant gets slightly slower (~10%), but the
array variant now takes 0.002s independent of the size of "m",
compared to some dozens of seconds without this patch!

Concerning your test case:
Your timeout of 2s is dangerously close to the timings of really fast
boxes without this patch, so I would lower this value.
I guess even on really slow ARM boxes or some-such this test case finishes
in some few tenth of seconds, at worst.

Or, as the new behavior seems to be independent of the m setting,
just bump the constant m by a factor 10 or 100. So you are sure no big iron
can pass this test without your patch being applied.

Thanks a bunch!
Manfred


> OK for trunk?
> 
> Regards,
> 
> Jerry
> 
> 2017-05-28  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> 
>     PR libgfortran/35339
>     * list_read.c.c (list_formatted_read_scala): Set the err return
>     value to the common.flags error values.

Re: [patch, libgfortran] PR53029 missed optimization in internal read (without implied-do-loop)

[Thomas Koenig]

Hi Jerry,

> Regression tested on x86_64. I have added a test case which will check 
> the execution time of the loop. The previous results of the REAd were 
> correct, just took a long time on large arrays.
> 
> OK for trunk?

OK.

It might be good if you followed Manfred's suggestion and turned
down the timeout to something like 0.5 seconds.

Thanks for the patch!

I would also consider backporting, the speedup is just so
large.  What do others think?

Regards

	Thomas

Re: [patch, libgfortran] PR53029 missed optimization in internal read (without implied-do-loop)

[Jerry DeLisle]

On 05/29/2017 09:51 AM, Thomas Koenig wrote:
> Hi Jerry,
> 
>> Regression tested on x86_64. I have added a test case which will check the 
>> execution time of the loop. The previous results of the REAd were correct, 
>> just took a long time on large arrays.
>>
>> OK for trunk?
> 
> OK.
> 
> It might be good if you followed Manfred's suggestion and turned
> down the timeout to something like 0.5 seconds.
> 
> Thanks for the patch!
> 
> I would also consider backporting, the speedup is just so
> large.  What do others think?
> 
> Regards
> 
>      Thomas

Committed.

	A	gcc/testsuite/gfortran.dg/read_5.f90
	M	gcc/testsuite/ChangeLog
	M	libgfortran/ChangeLog
	M	libgfortran/io/list_read.c
Committed r248577

Thanks,

Jerry