Closed Bug 1031198 Opened 10 years ago Closed 10 years ago

SIMD: Rename XXX.toYYY functions into YYY.fromXXX

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: bbouvier, Assigned: ProgramFOX, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits in SIMD.h and updated and renamed test cases in js/src/tests
10.55 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review 
The SIMD straw man proposal for JS changed recently [1]. In particular:
- X4.toY4 functions (e.g. Float32x4.toInt32x4) have been moved and renamed into Y4.fromX4 functions (e.g., in this case Int32x4.fromFloat32x4).
- ditto for X4.bitsToY4 (e.g. Float32x4.bitsToInt32x4 to Int32x4.fromFloat32x4Bits).

This bug consists in applying these modifications in our codebase. One should look at js/src/builtin/SIMD.h (and cpp), understand what the macros do and change the code there.

Tests in js/src/tests should also be modified. See for instance http://dxr.mozilla.org/mozilla-central/search?q=bitsToInt32x4 that shows you all places where bitsToInt32x4 are used in the codebase, and modify the tests accordingly. You'll have to check that the js test suite runs well, with /js/src/tests/jstests.py

If you have any questions, don't hesitate to ask them here, by email (ben@mozilla.com) or on IRC on #jsapi or #ionmonkey. For the record, we assign people to bugs once they have uploaded a first patch on the bug, so feel free to directly work on it and upload your patch, if nobody else did beforehand :)

[1] https://github.com/johnmccutchan/ecmascript_simd/commit/8941215c41fa0e93988768feb9be4799390724da
I renamed the functions X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits. I also updated the tests, which I will add in other attachments because I can only attach one file.
Attachment #8447610 - Flags: review?(benj)

        Attachment #8447610 -
        Attachment is obsolete: true

        Attachment #8447610 -
        Flags: review?(benj)

    
  

        Attachment #8447614 -
        Flags: review?(benj)

    
    

    
  
When trying to run the tests (using ./mach jstestbrowser), it performed some tests and then it gave an error, "recipe for target 'jstestbrowser' failed". And when I tried to run it from jstests.py (using the command python js/src/tests/jstests.py /obj-i686-pc-mingw32/js/src/shell/js), I got the error ""The program can't start because is nss3.dll is missing from your computer. Try reinstalling the program to fix this problem."

    
    

    
  
Pushed it to try since ProgramFOX is having trouble running the js tests locally.

https://tbpl.mozilla.org/?tree=Try&rev=369c557f9097

    
    

    
  
We have a test failure

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/jsreftest/tests/jsreftest.html?test=ecma_6/TypedObject/simd/int32x4bitstofloat32x4.js | Unknown file:///builds/slave/test/build/tests/jsreftest/tests/ecma_6/TypedObject/simd/int32x4bitstofloat32x4.js:14: TypeError: SIMD.float32x4.fromFloat32x4Bits is not a function item 1

https://tbpl.mozilla.org/?tree=Try&rev=8210c01853bb

    
    

    
  
(In reply to Manish Goregaokar [:manishearth] from comment #6)
> We have a test failure
> 
> REFTEST TEST-UNEXPECTED-FAIL |
> file:///builds/slave/test/build/tests/jsreftest/tests/jsreftest.
> html?test=ecma_6/TypedObject/simd/int32x4bitstofloat32x4.js | Unknown
> file:///builds/slave/test/build/tests/jsreftest/tests/ecma_6/TypedObject/
> simd/int32x4bitstofloat32x4.js:14: TypeError:
> SIMD.float32x4.fromFloat32x4Bits is not a function item 1
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8210c01853bb

Thanks, that should have been fromInt and not fromFloat. I'll upload a new patch.

    
    

    
  
Comment on attachment 8447827 [details] [diff] [review]
Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits and updated test cases; fixed test case error in previous (now obsolete) patch

Review of attachment 8447827 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice, thanks for this contribution!

Could you please add more context to your patch? (such as your author name, commit message -- see also [1] for examples of good commit messages). In the meanwhile, I'll send that to our try infrastructure, and once your new patch is uploaded, we can land it directly and it will be into Firefox 33. Nice work!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

        Attachment #8447827 -
        Flags: review?(benj) → review+

    
    

    
  
Comment on attachment 8447827 [details] [diff] [review]
Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits and updated test cases; fixed test case error in previous (now obsolete) patch

Review of attachment 8447827 [details] [diff] [review]:
-----------------------------------------------------------------

Also forgot: can you update all the test files names (but bug953270) to refer to the new function names? Thanks!

    
    

    
  
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Comment on attachment 8447827 [details] [diff] [review]
> Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits and updated
> test cases; fixed test case error in previous (now obsolete) patch
> 
> Review of attachment 8447827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also forgot: can you update all the test files names (but bug953270) to
> refer to the new function names? Thanks!

How can I put the renames in a diff file? hg diff shows only the file changes, not the renames.

    
    

    
  
(In reply to ProgramFOX from comment #12)
> 
> How can I put the renames in a diff file? hg diff shows only the file
> changes, not the renames.

If you use |hg mv old_name new_name| in CLI, it is automatically taken into account. Otherwise, you might have to |hg rm old_name|, |hg add new_name|, but I am unsure about that, although that's the way it'd work with git.

    
    

    
  
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)

Here is more context for the patch:

 - Author name: ProgramFOX
 - Commit message: Bug 1031198: Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits in SIMD.h and updated test cases in js/src/tests

    
    

    
  
(In reply to ProgramFOX from comment #14)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> 
> Here is more context for the patch:
> 
>  - Author name: ProgramFOX
>  - Commit message: Bug 1031198: Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4
> and Y4.fromY4Bits in SIMD.h and updated test cases in js/src/tests

Thanks for adding this! You might want to give a look at [1], as it explains how to set up your author name so that it's automatically included in your patch. For the commit message, you can use |hg qref -e| (if you're using the mq extension) and it will open a text editor for you. Let me know if you need help for doing this (in the worst case, I'll do it on your behalf :)). However, this is useful to know for the future, in particular if you are interested in continuing contributing afterwards :)

[1] https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Basic_configuration

    
    

    
  
(In reply to Benjamin Bouvier [:bbouvier] from comment #15)
> (In reply to ProgramFOX from comment #14)
> > (In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> > 
> > Here is more context for the patch:
> > 
> >  - Author name: ProgramFOX
> >  - Commit message: Bug 1031198: Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4
> > and Y4.fromY4Bits in SIMD.h and updated test cases in js/src/tests
> 
> Thanks for adding this! You might want to give a look at [1], as it explains
> how to set up your author name so that it's automatically included in your
> patch. For the commit message, you can use |hg qref -e| (if you're using the
> mq extension) and it will open a text editor for you. Let me know if you
> need help for doing this (in the worst case, I'll do it on your behalf :)).
> However, this is useful to know for the future, in particular if you are
> interested in continuing contributing afterwards :)
> 
> [1]
> https://developer.mozilla.org/en-US/docs/
> Installing_Mercurial#Basic_configuration

If I try the hg export command, it generates a patch and shows the differences between the two most recent commits. But I made two commits, so it shows the differences between the wrong commits. How can I show the differences between my latest commit and the commit before my first one?

    
    

    
  
You should really be using the Mercurial Queues feature for this. Otherwise you'll end up with lots of commits.

    
    

    
  


  
This diff file is generated using hg diff, not using hg export, and I added the additional info manually. The only difference I see, when comparing with other patches on Bugzilla, is that I don't have added a "Parent" included in my patch file. I found the ID of the parent, but this ID is very short, compared with IDs in other bug patches, so I left it out. Let me know if I should add it in.

        Attachment #8447827 -
        Attachment is obsolete: true

        Attachment #8448048 -
        Flags: review?(benj)

    
    

    
  
Comment on attachment 8448048 [details] [diff] [review]
Renamed X4.toY4 and X4.bitsToY4 to Y4.fromX4 and Y4.fromY4Bits in SIMD.h and updated and renamed test cases in js/src/tests

Review of attachment 8448048 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! I've added it locally to my patch queue so that the parent changeset id gets updated, updated the commit message so that it's shorter and added the "r=bbouvier" part that says I've validated your changes. Will now push it to our build infrastructure, as the try build returned green (reds are infra errors, it seems):

https://hg.mozilla.org/integration/mozilla-inbound/rev/9abc7829bde3

For future reference, feel free to give a look at [1] and in particular [2], which will introduce you to MQ, a mercurial extension that makes it really easier to work with patches / bugzilla.

Once again, thank you a lot for this patch! If you're interested in making other contributions to the JS engine, you can have a look at bug 1003801 (look at the opened "blocked by" bugs), contact any of us on irc (on #jsapi -- my nickname is bbouvier) or by adding another comment here.

Thanks to manishearth for the try build, btw :)

[1] https://developer.mozilla.org/en-US/docs/Mercurial
[2] https://developer.mozilla.org/en-US/docs/Mercurial_Queues

        Attachment #8448048 -
        Flags: review?(benj) → review+
Assignee: nobody → programfox
Status: NEW → ASSIGNED

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9abc7829bde3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: