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 09 Jul 2019 03:32:33 PM UTC, comment #19: 

OK, maybe I don't understand well enough the basics of how the JIT compiler is supposed to work, so let's try to fix that problem first...

I was thinking that the JIT compiler worked on functions, but now I see it also works on loops, independent of function bodies.  For loops, it could use the current scope and stack frame to see whether a variable has been flagged as global or persistent.

If compiling a complete function, I'm not sure how that would work since at the beginning of a function evaluation the status of variables (global or persistent) is not known unless global and persistent are handled as parse-time declarations instead of executable statements as they are now.  It seems to me that to properly handle global and persistent with the JIT compiler we will need additional support from the parser and a change in the way that global and persistent declarations work.  Given recent changes in Matlab diagnostics for global and persistent declarations, it appears Matlab is also heading in that direction.

John W. Eaton <jwe>
Project Administrator
Mon 08 Jul 2019 11:21:42 PM UTC, comment #18: 

The JIT evaluator doesn't manipulate the outer stack, so the dispatching frame should correspond to the current frame when is_global and is_persistent are called. So, at least at the top level of scope in the JIT, those calls should work in the same as if they were interpreted right? It would probably make sense to maintain a reference to the current frame rather than rely on the current frame in the tree_evaluator.

Gene Harvey <gharveymn>
Mon 08 Jul 2019 07:57:46 PM UTC, comment #17: 

Global and persistent attributes for variables are still handled as run-time things in the interpreter, but for JIT compiling, I think we need to handle them as declarations that happen when the function is parsed.

I'm not sure how best to do this job currently, but the way the new is_global and is_persistent functions are used doesn't look right to me.  When those are called, does the current scope correspond to the function that is being compiled?  If so, does that match the current stack frame?

John W. Eaton <jwe>
Project Administrator
Mon 08 Jul 2019 07:03:16 PM UTC, comment #16: 

Probably the same problem. Here's the build error:

make[2]: *** [Makefile:31099: doc/interpreter/interpn.eps] Error 134
/bin/sh: line 1:  5416 Aborted                 (core dumped) /bin/sh run-octave --norc --silent --no-history --path /home/gene/workspace/octave/dev/octave/build/x86_64-pc-linux-gnu/debug-llvm-3.7.1/../../../source/doc/interpreter/ --eval "interpimages ('doc/interpreter/', 'interpft', 'eps');"

and here's the coredump backtrace from gdb:

#0  0x00007f8190370755 in raise () from /usr/lib/libc.so.6
#1  0x00007f819035b851 in abort () from /usr/lib/libc.so.6
#2  0x00007f819035b727 in __assert_fail_base.cold () from /usr/lib/libc.so.6
#3  0x00007f8190369026 in __assert_fail () from /usr/lib/libc.so.6
#4  0x00007f819531d000 in (anonymous namespace)::CommandLineParser::registerCategory (
    this=0x564d1990b280, cat=0x7f8196c6c4c0 <llvm::cl::GeneralCategory>)
    at /home/gene/workspace/llvm/llvm-3.7.1/source/lib/Support/CommandLine.cpp:197
#5  0x00007f819531d190 in llvm::cl::OptionCategory::registerCategory (
    this=0x7f8196c6c4c0 <llvm::cl::GeneralCategory>)
    at /home/gene/workspace/llvm/llvm-3.7.1/source/lib/Support/CommandLine.cpp:240
#6  0x00007f8148351d39 in llvm::X86ATTInstPrinter::printCustomAliasOperand(llvm::MCInst const*, unsigned int, unsigned int, llvm::raw_ostream&) () from /usr/lib/libLLVM-8.so
#7  0x00007f819c60b79a in call_init.part () from /lib64/ld-linux-x86-64.so.2
#8  0x00007f819c60b8a1 in _dl_init () from /lib64/ld-linux-x86-64.so.2
#9  0x00007f819c60f683 in dl_open_worker () from /lib64/ld-linux-x86-64.so.2
#10 0x00007f819046d3d9 in _dl_catch_exception () from /usr/lib/libc.so.6
#11 0x00007f819c60ef5e in _dl_open () from /lib64/ld-linux-x86-64.so.2
#12 0x00007f8190b6c34c in ?? () from /usr/lib/libdl.so.2
#13 0x00007f819046d3d9 in _dl_catch_exception () from /usr/lib/libc.so.6
#14 0x00007f819046d473 in _dl_catch_error () from /usr/lib/libc.so.6
#15 0x00007f8190b6cab9 in ?? () from /usr/lib/libdl.so.2
#16 0x00007f8190b6c3da in dlopen () from /usr/lib/libdl.so.2
#17 0x00007f81645d1070 in glPrimitiveBoundingBox () from /usr/lib/libGLX_mesa.so.0
#18 0x00007f81645be1c3 in ?? () from /usr/lib/libGLX_mesa.so.0
#19 0x00007f81645c25c6 in ?? () from /usr/lib/libGLX_mesa.so.0
#20 0x00007f81645b3353 in ?? () from /usr/lib/libGLX_mesa.so.0
#21 0x00007f81645aef63 in ?? () from /usr/lib/libGLX_mesa.so.0
#22 0x00007f81645b0734 in ?? () from /usr/lib/libGLX_mesa.so.0
#23 0x00007f818f53a702 in glXChooseFBConfig () from /usr/lib/libGLX.so.0
#24 0x00007f817001a66a in ?? ()
   from /usr/lib/qt/plugins/xcbglintegrations/libqxcb-glx-integration.so
#25 0x00007f817001717f in ?? ()
   from /usr/lib/qt/plugins/xcbglintegrations/libqxcb-glx-integration.so
#26 0x00007f8170015a4a in ?? ()
   from /usr/lib/qt/plugins/xcbglintegrations/libqxcb-glx-integration.so
#27 0x00007f817c77b63e in QXcbIntegration::createPlatformOffscreenSurface(QOffscreenSurface*) const
    () from /usr/lib/libQt5XcbQpa.so.5
#28 0x00007f8199c43d4e in QOffscreenSurface::create() () from /usr/lib/libQt5Gui.so.5
#29 0x00007f81725843c7 in QtHandles::GLCanvas::begin_rendering (this=0x564d19c01b60)
--Type <RET> for more, q to quit, c to continue without paging--
    at ../../../source/libgui/graphics/GLCanvas.cc:280
#30 0x00007f8172583937 in QtHandles::GLCanvas::do_print (this=0x564d19c01b60, file_cmd=...,
    term=..., handle=...) at ../../../source/libgui/graphics/GLCanvas.cc:145
#31 0x00007f817258169c in QtHandles::Canvas::print (this=0x564d19c01b90, file_cmd=..., term=...)
    at ../../../source/libgui/graphics/Canvas.h:59
#32 0x00007f817257e10c in QtHandles::Figure::print (this=0x564d19bb1860, file_cmd=..., term=...)
    at ../../../source/libgui/graphics/Figure.cc:307
#33 0x00007f817258d4fd in QtHandles::Object::slotPrint (this=0x564d19bb1860, file_cmd=...,
    term=...) at ../../../source/libgui/graphics/Object.cc:137
#34 0x00007f81725b7b8c in QtHandles::Object::qt_static_metacall (_o=0x564d19bb1860,
    _c=QMetaObject::InvokeMetaMethod, _id=4, _a=0x7f8173feb520)
    at libgui/graphics/moc-Object.cc:102
#35 0x00007f81997f7d3a in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#36 0x00007f819a255485 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
   from /usr/lib/libQt5Widgets.so.5
#37 0x00007f819a25e9b1 in QApplication::notify(QObject*, QEvent*) ()
   from /usr/lib/libQt5Widgets.so.5
#38 0x00007f819c3ec15d in octave::octave_qapplication::notify (this=0x564d1998b0d0,
    receiver=0x564d19bb1860, ev=0x7f8168b98500) at ../../../source/libgui/src/octave-qobject.cc:67
#39 0x00007f81997cb782 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ()
   from /usr/lib/libQt5Core.so.5
#40 0x00007f81997ce459 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) ()
   from /usr/lib/libQt5Core.so.5
#41 0x00007f8199823c64 in ?? () from /usr/lib/libQt5Core.so.5
#42 0x00007f818f7e190f in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#43 0x00007f818f7e3869 in ?? () from /usr/lib/libglib-2.0.so.0
#44 0x00007f818f7e38ae in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#45 0x00007f8199823263 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#46 0x00007f81997ca3ac in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
   from /usr/lib/libQt5Core.so.5
#47 0x00007f81997d2056 in QCoreApplication::exec() () from /usr/lib/libQt5Core.so.5
#48 0x00007f819c3ec96d in octave::base_qobject::exec (this=0x7fff1b4a7910)
    at ../../../source/libgui/src/octave-qobject.cc:188
#49 0x00007f819c3f7c3b in octave::qt_application::execute (this=0x7fff1b4a79d0)
    at ../../../source/libgui/src/qt-application.cc:71
#50 0x0000564d194d2620 in main (argc=15, argv=0x7fff1b4a7cd8)
    at ../../../source/src/main-gui.cc:103

This is different than the one I'm referring to, but the principle is still there---libLLVM-8 used by Qt is trying to use libLLVM-3.7.1.

Gene Harvey <gharveymn>
Mon 08 Jul 2019 06:25:13 PM UTC, comment #15: 

What kind of problem do you run into when doing a full build with older version of LLVM ?

I recall that I had to build without the doc in some cases, are you referring to the same problem ?

Julien Bect <jbect>
Mon 08 Jul 2019 04:33:53 PM UTC, comment #14: 

@Julien Sorry about the wait, it turned out to be a bit more convoluted than expected. I've attached a couple changesets which should fix the behavior described.

fixincludes.patch: This adds HAVE_ macros to surround the includes. I had to define a new m4 macro which checks the exact location of a header to prevent shadowing from system headers. It really isn't the most lovely of solutions, but it seems to work.

Honestly, I'm not sure if I'm wasting my time hacking on that configuration code; I just don't understand how we intend to ship like this? If we did then we're locking down our users to one version of LLVM, and the fact that we also link to Qt5 (which uses LLVM-8) in libinterp will cause problems. I can't even do a full build when building with older versions of LLVM because for some reason the Qt code will include headers from that older version rather than LLVM-8, and this causes segfaults in the Qt code.

I really think we should just provide a jit_ir interface which dldfcns can hook into, the dldfcns will provide the translation to a single version of LLVM each. No convoluted configuration needed.

oldcrash.patch: This fixes the `LLVM_ERROR: ` crash. It turns out that it was an "Error: success" error from LLVM. I believe it was caused by the fact that we didn't declare functions in modules which they were used, but not defined. This is tested on LLVM debug builds of versions 3.7.1, 3.9.1, 4.0.1, 5.0.2, 6.0.1, 7.1.0, and 9.0.0svn, and then 8.0.0 release of course, all running on Arch x64.

(file #47170, file #47171)

Gene Harvey <gharveymn>
Mon 08 Jul 2019 02:17:36 PM UTC, comment #13: 

OK, I'll take a look at it.  It would be good to be able to build with the JIT compiler enabled.  We are currently building LLVM 6 for the llvmpipe OpenGL renderer for the Windows binaries.  I don't know whether there are specific requirements that force us to use version 6 for Mesa OpenGL, so maybe we could also use LLVM 8 for that?

John W. Eaton <jwe>
Project Administrator
Mon 08 Jul 2019 09:54:44 AM UTC, comment #12: 

@Gene: Concerning the autoconf issue  (comments #7, #8 and #9), are you going to fix it or do you want me to do it ?

@John, @Rik: The patch proposed by Gene makes it possible to build with --enable-jit on LLVM 8 and 9, and solves several open issues (see comment #5 and following).  I suggest to consider it for default.

Julien Bect <jbect>
Wed 26 Jun 2019 01:08:25 PM UTC, comment #11: 

Same configuration as in comment #5, this time with LLVM 9 (from Debian experimental).

Result: OK, same as LLVM 8.

Julien Bect <jbect>
Wed 26 Jun 2019 08:42:22 AM UTC, comment #10: 

Concerning the crash with LLVM 7: four unit tests are actually crashing Octave (the one reported below and three others).

All of them are related to testing indexing with parentheses.

Perhaps something to do with your changes to octave_jit_paren_scalar ?

Julien Bect <jbect>
Wed 26 Jun 2019 06:51:35 AM UTC, comment #9: 

Following up on comments #6 and #7: the function that you are looking for appears to be located in llvm/Transforms/Scalar.h in LLVM < 7.

Julien Bect <jbect>
Wed 26 Jun 2019 12:08:17 AM UTC, comment #8: 

Hey Julien, thanks for the feedback.

Comment #6: I just tried it with 7.0; same error. I also saw the same thing happening with 5.0. It seems to occur when calling llvm::getFunctionAddress (which occurs in jit_module::getFunctionAddress if you want to set a breakpoint). It's kind of ridiculous that LLVM calls `exit 1` after giving us no information. I, or someone smarter than me, might want to build from source to step through and see what the problem is.

Comment #7: I'm having some trouble compiling with 6.0 (some weird sigsegv when processing the eps files---probably on my end), but it's needed for llvm::createPromoteMemoryToRegisterPass in newer versions. I think the include was shadowed by my system include, so I neglected to autoconf it. I'm busy tonight, but I'll see if I can do that tomorrow.

I just want to add here that I'm currently working on a rework of the system. One of my primary goals is to decouple LLVM from the current codebase and link it through DEFUNs. Translation to LLVM IR can be done by a jit_ir visitor in the linked library instead of infecting the rest of the system. Dynamically linking the backend also allows us to support multiple versions of LLVM, and possibly other JITs like GNU JIT. I'm shooting for completion by the end of the summer, but I can't give any guarantees on that.

I'll also note that I'm finding the current jit_ir system difficult to debug and understand with all the templated internal lists and lack of documentation. The rework should provide a simpler system so we can hook in static analyzers (like use-before-def, which we can reflect in the editor).

Gene Harvey <gharveymn>
Tue 25 Jun 2019 06:35:53 PM UTC, comment #7: 

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

This time build fails:

  CXX      libinterp/parse-tree/libparse_tree_la-pt-jit.lo
libinterp/parse-tree/pt-jit.cc:127:10: fatal error: llvm/Transforms/Utils.h: No such file or directory
#include <llvm/Transforms/Utils.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

It seems that Utils.h does not exist in LLVM < 7, what is it needed for?

Julien Bect <jbect>
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 #47170:  includefix.patch added by gharveymn (9KiB - application/octet-stream)
file #47171:  oldcrash.patch added by gharveymn (39KiB - application/octet-stream)
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 jwe (Posted a comment)
  • -email is unavailable- added by jbect
  • -email is unavailable- added by jbect
  • -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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-07-08 gharveymn Attached File- => Added includefix.patch, #47170
        Attached File- => Added oldcrash.patch, #47171
    2019-07-08 jbect Carbon-Copy- => Added jwe@octave.org
        Carbon-Copy- => Added rik@octave.org
    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.5