patchGNU Octave - Patches: patch #10362, Fix problem with trivial ranges in...

 
 

patch #10362: Fix problem with trivial ranges in VM

Submitter:  Petter <petter>
Submitted:  Sat 24 Jun 2023 11:58:22 PM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  Ready For Test Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 04 Aug 2023 05:42:48 PM UTC, comment #10: 

Oops. Forgot to save the file before updating the commit. The style changes didn't make it into the first change as intended.

Pushed a follow-up here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9fddcbde0329

Markus Mützel <mmuetzel>
Group administrator
Fri 04 Aug 2023 05:26:35 PM UTC, comment #9: 

Sorry for taking so long.

I agree with jwe that it might be better to handle for-loop expressions different from ranges elsewhere. And eventually remove ranges entirely in favor of vectors.
But IIUC, this change fixes an issue in the current implementation. Fixing that now in the way Petter proposed seems trivial compared to changing the interpreter like jwe proposed even if I like jwe's proposition better.
So, I pushed the patch here after some minor changes to coding style and the commit message:
https://hg.savannah.gnu.org/hgweb/octave/rev/355b4434b45d

Marking as ready for test.


Markus Mützel <mmuetzel>
Group administrator
Sun 30 Jul 2023 08:27:34 AM UTC, comment #8: 

Should I change the patch somehow?

I would prefer changing type or removing ranges in the VM in another patch to keep this one focused on fixing the bug.

Petter <petter>
Tue 18 Jul 2023 10:48:06 PM UTC, comment #7: 

'Instead, could we handle "EXP : EXP" and "EXP : EXP : EXP" as special cases in the interpreter instead of creating range objects?  Then it seems to me that we could handle integer base, increment, and limit values in FOR loops without needing the special data type.'

Construct like this one bellow maybe lead to problems with memory allocation if there is no lazy range object?


function foo
  for a = some_range()
  end
end

function r = some_range
  r = 1:1e9;
end


'Again, I encourage you to not focus so much on these kinds of issues until the bytecode interpreter is fully functional.'

Ye I agree. This patch is a bugfix and I explained why "trivial range" was done in the first place, i.e. for performance reasons.

Petter <petter>
Tue 18 Jul 2023 05:08:57 AM UTC, comment #6: 

"Also I believe int:s are faster than long long, but I haven't verified that."

Again, I encourage you to not focus so much on these kinds of issues until the bytecode interpreter is fully functional.

John W. Eaton <jwe>
Group administrator
Tue 18 Jul 2023 05:06:05 AM UTC, comment #5: 

I'm not convinced that we need a special data type for this job. 

Instead, could we handle "EXP : EXP" and "EXP : EXP : EXP" as special cases in the interpreter instead of creating range objects?  Then it seems to me that we could handle integer base, increment, and limit values in FOR loops without needing the special data type.

See also the various discussions we have had about eliminating the special range data type.  Matlab doesn't have one, so we don't need it in Octave for compatibility and it has caused a lot of trouble and confusion in the past.

John W. Eaton <jwe>
Group administrator
Mon 17 Jul 2023 10:49:34 PM UTC, comment #4: 

Should I do another patch for making it octave_idx_type type instead of ints, or add it to this bugfix?

The "trivial range" is used to speed up for-loops in the VM, cutting out a check for "reversed" ranges as-well as not doing floating-point math.

With the posted patch there should be correct behavior, i.e. emulate the ordinary double ranges but with integer math, for a subset of ranges that are suitable. I overlooked actually checking if the base, increment etc are integers in the original code.

The added benefit of using 'octave_idx_type' over int would be that integer CPU operations are used for bigger ranges.

Petter <petter>
Mon 17 Jul 2023 06:29:08 AM UTC, comment #3: 

In case this depends on whether an integer is representable as a double precision number, 2^53 might not be the universal upper limit. IIUC, it can be larger. E.g., if the base is a multiple of 2^N and the increment is 2^N, the integers can be represented correctly up to 2^(53+N).

Anyway, the "easy way out" would probably be to just use `octave_idx_type` here. Afaict, that is `int64_t`. That should be `long` on Linux or `long long` on Windows.
Imho, correct behavior might be more important than performance in this case.

Markus Mützel <mmuetzel>
Group administrator
Sun 16 Jul 2023 09:54:54 PM UTC, comment #2: 

There would be some thinking required to find out which final value of the range will give the same values for double-math and long long-math for each element along the way. I think it might be 2^53, the last integer of which the prior integer could be represented exactly in a double, but I was not too sure and chose the easy way out by using int limits instead.

Also I believe int:s are faster than long long, but I haven't verified that.

Petter <petter>
Sat 15 Jul 2023 11:59:41 AM UTC, comment #1: 

Thank you for the patch.

Is there a particular reason why the "trivial range" is limitted to `int`? Would it make sense to use `octave_idx_type` for the range base and increment instead?

Markus Mützel <mmuetzel>
Group administrator
Sat 24 Jun 2023 11:58:22 PM UTC, original submission:  

"
Fix problem with trivial ranges in VM

  • libinterp/octave-value/ov-range.cc: Check the doubles are integers, in range
  • libinterp/octave-value/ov-range.h


  • test/compile/bytecode.tst: Added tests
  • test/compile/bytecode_for.m

"

VM specific 'trivial_range' for for-loops did not work properly for non-integer steps, which was discovered by running "pkg test stk".

Petter <petter>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54868:  octave_32270.patch added by petter (4KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by petter (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-08-04 mmuetzel StatusNeed Info Ready For Test
    2023-07-15 mmuetzel CategoryNone Core : other
        StatusNone Need Info
    2023-06-24 petter Attached File- Added octave_32270.patch, #54868

    Back to the top

    Powered by Savane 3.14-f13d.
    Corresponding source code