bugGNU Octave - Bugs: bug #56425, Patch for the JIT

 
 

bug #56425: Patch for the JIT

Submitted by:  Gene Harvey <gharveymn>
Submitted on:  Sat 01 Jun 2019 03:34:19 AM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  None Assigned to:  None
Originator Name:  Gene Harvey Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 25 Jun 2019 12:48:00 PM UTC, comment #6:

Same configuration as in comment #5, but with LLVM 7.0.

Build succeeds but jit.tst crashes octave:

***** testif HAVE_LLVM
jit_failcnt (0);
ndim = 100;
result = 0;
m = zeros (ndim);
m(:) = 1:ndim^2;
i = 1;
while (i <= ndim)
   for j = 1:ndim
     result = result + m(i, j);
    endfor
   i = i + 1;
endwhile
assert (result == sum (sum (m)));
assert (jit_failcnt, 0);
LLVM ERROR:

@Gene: Have you seen something similar with LLVM 7.0 ?

Julien Bect <jbect>
Tue 25 Jun 2019 07:19:44 AM UTC, comment #5:

I have built Octave default (30f53f7a7293184f38b832a6494901636f5f2015) with the four patches applied, on Debian unstable with LLVM 8.0.

-----

1) The build succeeds and here is the result of make check:

                                                    (reported bug) XFAIL   2
Summary:
fixed/jit.tst .................................................. PASS   36/38 

  PASS                            15604
  FAIL                                1
  XFAIL (reported bug)               27
  SKIP (missing feature)              9
  SKIP (run-time condition)          34

So, this patch fixes bug #55809: "Building with --enable-jit fails since the recent variable storage refactoring".

-----

2) The following snippet from bug #55492, comment #2, no longer crashes octave:

jit_enable (1)
m = [1 2; 3 4];
while (1)
  z = m(1, 2);
  break;
endwhile

The original bug report was for LLVM 4.0, which I haven't tested yet, but apart from that I would that this patch fixes bug #55492 too.

-----

3) The following snippet from bug #55469, comment #2, no longer crashes octave:

jit_enable (true);
jit_startcnt (5);

x = 1 + 1i;
for j = 1:5
    x = x + 1;
endfor

So, it appears that this patch also fixes bug #55469 ;-)

-----

4) Also, the following snippet from bug #53691 no longer crashes octave:

jit_enable (1)
f = inline ("x^2 + 2");
f(1)

So, it appears that this patch fixes bug #53691 too !

-----

Summary: this patch makes it possible to build Octave with LLVM 8 and solves four open issues.

I haven't carried out a detailed code review, nor will I have time to do it in the near future, but from an "external" point of view I would vote for pushing this to default.

Julien Bect <jbect>
Tue 04 Jun 2019 09:21:26 PM UTC, comment #4:

Alright, I've split the patch into four components and removed all the extraneous stuff. Hopefully it's more digestible now.

Contents:

  • jitconfigure.patch: A fix for builds breaking on some systems because of the use of -isystem.
  • jitbuild.patch: Updates to work with LLVM-8.0 and the stack_frame refactor (bug #55809).
  • jitbug55492.patch: A fix for bug #55492. This is meant to be temporary.
  • jitsinfail.patch: A temporary workaround for a segmentation fault that occurs when calling `sin` with the JIT.

(file #47031, file #47032, file #47033, file #47034)

Gene Harvey <gharveymn>
Mon 03 Jun 2019 05:47:14 AM UTC, comment #3:

My fault, I left a lot of changes in that weren’t strictly necessary. I can definitely repackage it if people aren’t too far through yet.

With regards to the std::endl changes, it’s just premature optimization (std::endl flushes the buffer). So not really needed.

Gene Harvey <gharveymn>
Sun 02 Jun 2019 10:48:36 PM UTC, comment #2:

Also, afaict, this patch is still using the C++ interface to LLVM. That is unstable and they suggest we use the C interface instead. It became unsustainable to keep up with the C++ API changes to LLVM, so I think any attempt to revive Octave's JIT should be focussed on moving to LLVM's C API or to any other JIT library (e.g. libgccjit, GNU lightning or whatever else).

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Sun 02 Jun 2019 10:45:34 PM UTC, comment #1:

There seem to be a lot of unrelated changes here, what's the point of changing `os << std::endl;` to `os << '\n'`?

This patch is also kind of mammoth which makes it difficult to review. Think you could split it up?

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Sat 01 Jun 2019 03:34:19 AM UTC, original submission:

This patch resolves bug #55492 and bug #55809, and some other unlisted bugs. It also adds support for versions of LLVM up to and including LLVM-8.0. However, LLVM-5.0 seems to error in llvm::getFunctionAddress with empty string output to stderr and then calls `exit 1`. So if the patch gets accepted, I'll put that on the bug tracker.

Unlisted bugs fixed:

  • The build breaks on some systems if the LLVM include directory is /usr/include because when we use -isystem it reorders the search path.
  • Build error from incorrect signature for llvm::WriteBitcodeToFile.
  • Octave crashes when the JIT tries to call `sin` (although, I'm still not totally sure what's wrong there).
Gene Harvey <gharveymn>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47031:  jitconfigure.patch added by gharveymn (4KiB - text/x-patch)
file #47032:  jitbug55492.patch added by gharveymn (4KiB - text/x-patch)
file #47033:  jitbuild.patch added by gharveymn (24KiB - text/x-patch)
file #47034:  jitsinfail.patch added by gharveymn (1KiB - text/x-patch)
file #47009:  jitfix.patch added by gharveymn (106KiB - text/x-patch - or rev 1ccb99c08f27 on https://gharveymn@bitbucket.org/gharveymn/octave)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jbect (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by gharveymn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-06-04 gharveymn Attached File- => Added jitconfigure.patch, #47031
        Attached File- => Added jitbug55492.patch, #47032
        Attached File- => Added jitbuild.patch, #47033
        Attached File- => Added jitsinfail.patch, #47034
    2019-06-01 gharveymn Attached File- => Added jitfix.patch, #47009

    Back to the top


    Powered by Savane 3.4