bugGNU Octave - Bugs: bug #41560, Typo in jit-typeinfo.cc

 
 

bug #41560: Typo in jit-typeinfo.cc

Submitter:  None
Submitted:  Wed 12 Feb 2014 01:20:42 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  David Malcolm Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 13 Feb 2014 04:57:04 PM UTC, comment #4: 

I don't think it makes any difference, since compiling functions doesn't work at all. Just check by enable jit debugging with debug_jit (1)

There's a workaround patch in bug #41312 file functions_compile.patch

Stefan Mahr <dac922>
Wed 12 Feb 2014 05:40:44 PM UTC, comment #3: 

I tested JIT before and after the change and it was definitely calling the exp function.  I ran


jit_enable (1)
jit_startcnt (1)
x = [1:5];
for i = 1:5
   y(i) = exp (x(i));
endfor


The first two lines are boilerplate to engage the JIT compiler.

The value of y was equivalent to exp ([1:5]) before and after the typo fix.  At any rate, this was surely the right thing to do.

Rik <rik5>
Group administrator
Wed 12 Feb 2014 05:15:19 PM UTC, comment #2: 

FWIW, I think the issue was more serious: the parameter was passed as the 2nd param to register_intrinsic, aka "iid":
http://hg.savannah.gnu.org/hgweb/octave/file/634d9989bf7b/libinterp/corefcn/jit-typeinfo.cc#l2047
where it gets converted to :
  llvm::Intrinsic::ID id = static_cast<llvm::Intrinsic::ID> (iid);

and then used in this call:
  llvm::Function *ifun = llvm::Intrinsic::getDeclaration (module, id, llvm_args);

Hence I believe that calls to "exp" were being erroneously jit-compiled to "cos" calls, thus silently giving the wrong answers.

That said, your fix in 634d9989bf7b looks good; thanks!

David Malcolm <dmalcolm>
Wed 12 Feb 2014 04:39:36 PM UTC, comment #1: 

Luckily, that second parameter seems to be a size parameter and the return size of cos (double) is the same as exp (also double).  Nevertheless, I corrected it in this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/634d9989bf7b).

Rik <rik5>
Group administrator
Wed 12 Feb 2014 01:20:42 AM UTC, original submission:  

In http://hg.savannah.gnu.org/hgweb/octave/file/1363d909c577/libinterp/corefcn/jit-typeinfo.cc#l1848:

  register_intrinsic ("exp", llvm::Intrinsic::cos, scalar, scalar);

Note the use of "cos" in the 2nd argument, which appears to be a copy-and-paste error from a similar registration a few lines above.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dac922 (Posted a comment)
  • -email is unavailable- added by dmalcolm (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (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 group members can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-02-12 rik5 StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code