Bug 152784 - Add new or32 implementation to MacroAssemblerARM after r194613
Summary: Add new or32 implementation to MacroAssemblerARM after r194613
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 108645 132470
  Show dependency treegraph
 
Reported: 2016-01-06 03:31 PST by Csaba Osztrogonác
Modified: 2016-01-07 07:31 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.53 KB, patch)
2016-01-06 03:33 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (1.67 KB, patch)
2016-01-07 03:20 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2016-01-06 03:31:31 PST
https://trac.webkit.org/changeset/194613 added the new or32 
to ARMv7(Thumb2) and AArch64 assemblers, but not to ARMv7(ARM).
Comment 1 Csaba Osztrogonác 2016-01-06 03:33:53 PST
Created attachment 268365 [details]
Patch
Comment 2 Benjamin Poulain 2016-01-06 04:18:24 PST
Comment on attachment 268365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268365&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:231
> +    void or32(TrustedImm32 imm, AbsoluteAddress dest)

Uh, that sucks :(

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:235
> +        or32(imm, ARMRegisters::S1);

I don't think that's fine. It will override S0 to load imm into register.
Comment 3 Csaba Osztrogonác 2016-01-06 04:44:52 PST
(In reply to comment #2)
> Comment on attachment 268365 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268365&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:231
> > +    void or32(TrustedImm32 imm, AbsoluteAddress dest)
> 
> Uh, that sucks :(
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:235
> > +        or32(imm, ARMRegisters::S1);
> 
> I don't think that's fine. It will override S0 to load imm into register.

It is a copy/paste of the ARMv7(Thumb2) change:
https://trac.webkit.org/changeset/194613/trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

And JSC stress tests run without regression.
Comment 4 Benjamin Poulain 2016-01-06 04:57:18 PST
(In reply to comment #3)
> > I don't think that's fine. It will override S0 to load imm into register.
> 
> It is a copy/paste of the ARMv7(Thumb2) change:
> https://trac.webkit.org/changeset/194613/trunk/Source/JavaScriptCore/
> assembler/MacroAssemblerARMv7.h

The thumb version of or32() uses the "dataTempRegister" to load the immediate. It does not override the address.

The ARM version you propose override the address.

> And JSC stress tests run without regression.

Looks like the test coverage is not good enough then. Please add a test.
Comment 5 Csaba Osztrogonác 2016-01-07 03:20:20 PST
Created attachment 268449 [details]
Patch

Thanks for catching this bug around or32(imm,reg). I think the easiest fix is reloading the address to S0 after or32(imm,reg) overwrote it. Let me know if you have better idea to fix this issue.
Comment 6 Csaba Osztrogonác 2016-01-07 03:22:27 PST
(In reply to comment #4)
> (In reply to comment #3)

> > And JSC stress tests run without regression.
> Looks like the test coverage is not good enough then. Please add a test.

It's not trivial at all to write a test in JS to trigger a behaviour
in macroassembler and there is no test framework for macroassembler,
so I don't think if it is a fair request.
Comment 7 Benjamin Poulain 2016-01-07 04:51:28 PST
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> 
> > > And JSC stress tests run without regression.
> > Looks like the test coverage is not good enough then. Please add a test.
> 
> It's not trivial at all to write a test in JS to trigger a behaviour
> in macroassembler and there is no test framework for macroassembler,
> so I don't think if it is a fair request.

You would need a stress-test that exercices the baseline-JIT/DFG part that uses this form of or32.

I understand you don't want to do that.

(In reply to comment #5)
> Created attachment 268449 [details]
> Patch
> 
> Thanks for catching this bug around or32(imm,reg). I think the easiest fix
> is reloading the address to S0 after or32(imm,reg) overwrote it. Let me know
> if you have better idea to fix this issue.

IMHO, that's perfectly reasonable.
Comment 8 Csaba Osztrogonác 2016-01-07 07:31:42 PST
Comment on attachment 268449 [details]
Patch

Clearing flags on attachment: 268449

Committed r194698: <http://trac.webkit.org/changeset/194698>
Comment 9 Csaba Osztrogonác 2016-01-07 07:31:51 PST
All reviewed patches have been landed.  Closing bug.