Remove occasional redundant Push/Pop in FlexibleShift helper

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: mgaudet, Assigned: kellykc72, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

In Bug 1526024 the FlexibleShift helper was rewritten to fix correctness, with tests added.

A side effect of the new helper however is that when shift is already in register ecx, we'll generate a push/pop pair that are redundant, as that register already will not get clobbered.

This bug is about making that push conditional.

A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift. A bonus would be if you used masm.breakpoint() and a debugger to inspect the emitted instructions to verify the push and pop are elided.

Priority: -- → P3

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)

In Bug 1526024 the FlexibleShift helper was rewritten to fix correctness, with tests added.

A side effect of the new helper however is that when shift is already in register ecx, we'll generate a push/pop pair that are redundant, as that register already will not get clobbered.

This bug is about making that push conditional.

A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift. A bonus would be if you used masm.breakpoint() and a debugger to inspect the emitted instructions to verify the push and pop are elided.

Hi, This was tagged 'Good-first-bug', I was hopping I could be assigned to it, but I do not see the 'Take' button. Please advice

Flags: needinfo?(mgaudet)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Thank you very much, I'll take a look at these resources! Best Regards

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Also, thank you for explaining :)

[Update:] Successfully built Firefox for Desktop. Proceeding to build SpiderMonkey

Hey kellykc72, sounds like you're started up. Let me know if you have questions.

This is indeed a slightly more challenging bug, but because it's in a test suite, I think it's not too bad compared to other JIT bugs :D

(And thanks so much Emanuel for jumping in on this!)

Flags: needinfo?(mgaudet)

Haha, challenges are always fun. Just getting around to completing the SpiderMonkey build. I'll keep you all posted!

SpiderMonkey Build completed. I shall proceed to use searchfox to find the associated code. I'll ping back if i need more information. Thanks

Checking that inline void FlexibleShift32(MacroAssembler&, Register, Register, bool, bool) is the function to be fixed. Is there a 64bit version? excuse me if that's a silly question.

You've got the right function!

Sweeet! I'll read up, get some context about the code, the bug what I'm thinking and you can verify and point me in the right direction. Thank you

Hey, so I wrapped preserve.add(ecx); in a condition. However, I want to confirm that this statement from your original report "A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift." means my fix must pass testJitMacroAssembler_flexible*shift before it will be accepted.

Flags: needinfo?(mgaudet)

In the meantime, I will research how to test and debug the JIT engine. Thank you

Yep. That's correct.

If you've not done so already, after you build the shell, there should be in dist/bin a jsapi-tests binary; you can run the shift tests by just doing ./dist/bin/jsapi-tests flexible (ok, I mean, that also runs flexibleDivMod and flexibleRemainder but these don't take too long).

It might be cool/educational (up to you) to use masm.breakpoint() to cause a trap to happen when the case you're fixing happens so you can look at the assembly code and see how it changes before and after your patch.

Flags: needinfo?(mgaudet)

Oh nice, I actually came here to ask about what you just answered! So I found the binary but in other to find the usage, I was searching for an entry point. I saw a main in tests.cpp, but from this command you just gave ./dist/bin/jsapi-tests flexible, it appears I had the wrong entry point. I guess the broader question will be 'are there any documentation on how the tests are structured/run?'

Oh yeah I was gonna do the breakpoint stuff. But before that. Please confirm my workflow, I feel its supposed to be a lot simpler.

  1. Modified the appropriate function
  2. Executed the configure --enable-debug --disable-optimize --enable-nspr-build --enable-build-backends=CompileDB,RecursiveMake I don't think I should have done this. Please confirm
  3. Executed mozmake I think this is all I should have needed to do, although it seemed to have done way more stuff than I was expecting (considering I have run it before and only made a little change)

Let me go checkout the masm.breakpoint stuff.

Flags: needinfo?(mgaudet)

Also, could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

(In reply to kellykc72 from comment #16)

Also, please could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

Regarding the configure options, I don't think you need the --enable-build-backends option. The others should be fine for debugging purposes - --disable-optimize makes it slow, but without it a lot of things will get optimized out.

One thing to note: as of fairly recently, Windows shell builds are 64-bit by default (I assume you're on Windows because you mentioned mozmake). If you're adjusting the x86 JIT backend you might need a cross-compile. Just adding --target=i686-pc-mingw32 might work, though if it complains about the host configuration being unknown you might need to add --host=x86_64-pc-mingw32 as well.

You can pass the -s flag to mozmake just like with make to make it a bit more quiet, and -jn with n a number to make it perform multiple jobs in parallel (e.g. -j6 for 6 parallel jobs). The console output will get a bit mangled unfortunately, but it should be a lot faster.

Oh, and you might already be doing this, but it's highly recommended to build in a subdirectory. So you'd navigate to js/src, mkdir mybuild, cd mybuild and then run configure as ../configure instead.

(In reply to kellykc72 from comment #16)

Also, could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

(Bearing in mind I don't have a lot of Windows development experience)

It should be just a matter of attaching to jsapi-tests with a debugger; when you execute the JIT compiled code that has a masm.breakpoint() in, most debuggers should stop there. At that point you're stopped in JIT compiled code, and so you don't get symbols or line numbers. You'd then want to check the assembly by using your debuggers disassembly instructions.

At this point it sounds like you've got a patch worth looking at: Follow the instructions here to submit a patch :D

Flags: needinfo?(mgaudet)

Using Visual Studio, what you can do is go to File, Open, Project/Solution... and open jsapi-tests.exe as though it were a project. Then you can adjust the project properties to set the arguments and click the Start button to start debugging.

Alternatively you can try running devenv.exe directly with the /DebugExe parameter, but it might not be in your %PATH% (it's located in an awkward location like %ProgramFiles(x86)%\Microsoft Visual Studio\2017\Community\Common7\IDE). If you want to try it, the correct incantation is devenv -DebugExe path/to/jsapi-tests.exe [parameters] (the .exe extension is required or -DebugExe will silently fail).

Thanks [:ehoogeveen] and [:mgaudet]. Let me play around with these suggestions.

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Flags: needinfo?(emanuel.hoogeveen)

Hi Matthew, I believe this is the behavior you are expecting? Please advice

shift in ECX (PUSH/POP pair ABSENT for ECX)

  00000350A6DA0092  mov         ecx,0FEEDBADh
  00000350A6DA0097  mov         eax,10h  
  00000350A6DA009C  mov         ecx,2  
  00000350A6DA00A1  int         3                            :BREAK

EAX = 16, ECX = 2 // in DECIMAL
  00000350A6DA00A2  shl         eax,cl                       :16 << 2
EAX = 64, ECX = 2
  00000350A6DA00A4  mov         r11,0CFFB1FFA68h             :*(0CFFB1FFA68h) is 64
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00AE  cmp         dword ptr [r11],eax          :*(0CFFB1FFA68h) == 64 <=> Equal
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00B1  jne         00000350A6DA00CF             :Jump not taken
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00B7  mov         r11,0CFFB1FFA70h             :*(0CFFB1FFA70h) is 2
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00C1  cmp         dword ptr [r11],ecx          :*(0CFFB1FFA70h) == 2 <=> Equal
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00C4  jne         00000350A6DA0338             :Jump not taken
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00CA  jmp         00000350A6DA0F58             :Unconditional Jump taken

Random value in ECX (PUSH/POP pair PRESENT for ECX)

  00000350A6DA0F58  mov         ecx,0FEEDBADh  
  00000350A6DA0F5D  mov         eax,10h  
  00000350A6DA0F62  mov         edx,2  
  00000350A6DA0F67  int         3                            :BREAK

EAX = 16, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F68  push        rcx                          :Save ECX (it'll be clobbered)
EAX = 16, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F69  mov         rcx,rdx                      :Overwrite RCX content with RDX
EAX = 16, EDX = 2, ECX = 2
  00000350A6DA0F6C  shl         eax,cl                       :16 << 2
EAX = 64, EDX = 2, ECX = 2
  00000350A6DA0F6E  pop         rcx                          :Reset ECX using saved value
EAX = 64, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F6F  mov         r11,0CFFB1FFA68h             :*(0CFFB1FFA68h) is 64
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F79  cmp         dword ptr [r11],eax          :*(0CFFB1FFA68h) == 64 <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F7C  jne         00000350A6DA0FAD             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F82  mov         r11,0CFFB1FFA70h             :*(0CFFB1FFA70h) is 2
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F8C  cmp         dword ptr [r11],edx          :*(0CFFB1FFA70h) == 2 <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F8F  jne         00000350A6DA1216             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F95  mov         r11,7FF7F1F8ABE8h            :*(7FF7F1F8ABE8h) is 0FEEDBADh 
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0F9F  cmp         dword ptr [r11],ecx          :*(7FF7F1F8ABE8h) == 0FEEDBADh <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0FA2  jne         00000350A6DA147F             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0FA8  jmp         00000350A6DA1E33             :Unconditional Jump taken
Flags: needinfo?(mgaudet)

(In reply to kellykc72 from comment #23)

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Actually I tried to compile with those options and it ended in chaos haha

(In reply to kellykc72 from comment #25)

(In reply to kellykc72 from comment #23)

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Actually I tried to configure with those options and it ended in chaos haha

I'm probably missing something but ...

In file included from d:/firefox/js/src\jit/LIR.h:1806d:/firefox/js/src\jit/LIR.h:
(715,d:/firefox/js/src\jit/x86/LIR-x86.h(209,6314))::  noteerror: : expanded fromno  macromember  'LIR_HEADER'named
 'WasmAtomicStoreI64'  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
in 'js::jit::LNode::Opcode'                                               ~~~~~~~~~~~~~~~^

  LIR_HEADER(WasmAtomicStoreI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/build_DBG.OBJ/js/src/wasm/Unified_cpp_js_src_wasm1.cpp:47:
In file included from d:/firefox/js/src/wasm/WasmIonCompile.cpp:23:
In file included from d:/firefox/js/src\jit/CodeGenerator.h:16:
In file included from d:/firefox/js/src\jit/x86/CodeGenerator-x86.h:10:
In file included from d:/firefox/js/src\jit/x86-shared/CodeGenerator-x86-shared.h:10:
In file included from d:/firefox/js/src\jit/shared/CodeGenerator-shared.h:16:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(284,14): error: no member named 'WasmAtomicBinopI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicBinopI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(233,14): error: no member named 'WasmCompareExchangeI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmCompareExchangeI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(261,14): error: no member named 'WasmAtomicExchangeI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicExchangeI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(284,14): error: no member named 'WasmAtomicBinopI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicBinopI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode

Oh I forgot the test output

$ ./dist/bin/jsapi-tests.exe flexible
testJitMacroAssembler_flexibleLshift
TEST-PASS | testJitMacroAssembler_flexibleLshift | ok
testJitMacroAssembler_flexibleRshiftArithmetic
TEST-PASS | testJitMacroAssembler_flexibleRshiftArithmetic | ok
testJitMacroAssembler_flexibleRshift
TEST-PASS | testJitMacroAssembler_flexibleRshift | ok
testJitMacroAssembler_flexibleQuotient
TEST-PASS | testJitMacroAssembler_flexibleQuotient | ok
testJitMacroAssembler_flexibleRemainder
TEST-PASS | testJitMacroAssembler_flexibleRemainder | ok
testJitMacroAssembler_flexibleDivMod
TEST-PASS | testJitMacroAssembler_flexibleDivMod | ok

Passed: ran 6 tests.

This code is shared between x86 and x86-64 backends, so no need to cross compile!

Flags: needinfo?(emanuel.hoogeveen)

I think the behaviour looks right. Definitely time to submit a patch :)

Flags: needinfo?(mgaudet)

sweeet!!

FWIW cross compilation seems to be broken for me at the moment, at least using mozmake and configure (mach seems okay). I'll have to file a bug, but no need to worry about it here.

Update: busy week(end), just getting around setting up Phabricator. Didn't know earlier, should've set it up with everything else before starting. Sorry for the delay

The ECX register must contain the SHIFT amount. Before this change, ECX will always be push/pop during a SHIFT operation. However, ECX should only be push/pop if it's original contents are going to be overwritten. We now only push/pop ECX if it contains some arbituary value and hence will be overwritten by the SHIFT operation.

Attachment #9049858 - Attachment description: make push/pop pair for ecx conditional on previous ecx content → Make push/pop pair for ecx conditional on previous ecx content

Thanks so much for your contribution! If you'd like to keep rolling, please, feel free to look for more mentored bugs in SpiderMonkey.

Perhaps give Bug 1531479 a look.

(Pulsebot would normally link to the landed code, but it seems to be sleeping right now, so:

https://hg.mozilla.org/integration/autoland/rev/5ec94981916f0660a2cd2348ef41a10ffc4fca93

)

Assignee: nobody → kellykc72

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #34)

Thanks so much for your contribution! If you'd like to keep rolling, please, feel free to look for more mentored bugs in SpiderMonkey.

Perhaps give Bug 1531479 a look.

(Pulsebot would normally link to the landed code, but it seems to be sleeping right now, so:

https://hg.mozilla.org/integration/autoland/rev/5ec94981916f0660a2cd2348ef41a10ffc4fca93

)

Thanks to you and :ehoogeveen for all the assistance and yes, I'll keep rolling. Let me take a look :)

Hi, please confirm no further action's are required from me for this. I read something about landing but I suppose you as the reviewer is responsible for that?

Flags: needinfo?(mgaudet)

Oh nvm, I guess that's the link you sent regarding Pulsebot. Thanks!

Flags: needinfo?(mgaudet)

You're all done. What's happening now is the code has landed on our integration branch 'autoland'; it's been tested through our continuous integration system; you can see the results here.

Next up, at some point in the not too distant future the Mozilla Sheriffs will merge autoland into mozilla-central, the repository from which our nightly builds are built. At that point, this bug will get marked as RESOLVED by the sheriff at the merge time.

Then, a few hours after that your code will be in a copy of Firefox Nightly that tens of thousands run every day :)

Party time: 🎉🎉🎉

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #38)

You're all done. What's happening now is the code has landed on our integration branch 'autoland'; it's been tested through our continuous integration system; you can see the results here.

Next up, at some point in the not too distant future the Mozilla Sheriffs will merge autoland into mozilla-central, the repository from which our nightly builds are built. At that point, this bug will get marked as RESOLVED by the sheriff at the merge time.

Then, a few hours after that your code will be in a copy of Firefox Nightly that tens of thousands run every day :)

Party time: 🎉🎉🎉

Haha, thanks for the clarification! Already looking forward to my next contribution!

(In reply to kellykc72 from comment #35)

Thanks to you and :ehoogeveen for all the assistance and yes, I'll keep rolling. Let me take a look :)

No problem, congrats on your first contribution! I'm glad you had a positive experience and I'm looking forward to seeing what you do in the future :)

Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.