Bug 153065 - [JSC] Legalize Memory Offsets for ARM64 before lowering to Air
Summary: [JSC] Legalize Memory Offsets for ARM64 before lowering to Air
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on: 153075 153232
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-13 09:06 PST by Benjamin Poulain
Modified: 2016-01-19 06:08 PST (History)
8 users (show)

See Also:


Attachments
Patch (18.39 KB, patch)
2016-01-13 09:10 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (19.04 KB, patch)
2016-01-13 10:25 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (18.42 KB, patch)
2016-01-13 10:33 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (16.56 KB, patch)
2016-01-15 15:40 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2016-01-13 09:06:18 PST
[JSC] Legalize Memory Offsets for ARM64 before lowering to Air
Comment 1 Benjamin Poulain 2016-01-13 09:10:24 PST
Created attachment 268870 [details]
Patch
Comment 2 WebKit Commit Bot 2016-01-13 09:12:00 PST
Attachment 268870 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:154:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:169:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2016-01-13 09:30:25 PST
Comment on attachment 268870 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        There are two available addressing: signed 9bits and unsigne scaled 12bits.

typo: "unsigne" ==> "unsigned".

> Source/JavaScriptCore/b3/B3LegalizeMemoryOffsets.h:36
> +// compute it explicitely.

typo: "explicitely" ==> "explicitly".

> Source/JavaScriptCore/b3/testb3.cpp:171
> +void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32)
> +{
> +    {
> +        Procedure proc;
> +        BasicBlock* root = proc.addBlock();
> +        int64_t x = -42;
> +        Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
> +        root->appendNew<ControlValue>(
> +            proc, Return, Origin(),
> +            root->appendNew<MemoryValue>(
> +                proc, Load, Int64, Origin(),
> +                base,
> +                offset64));
> +
> +        char* address = reinterpret_cast<char*>(&x) - offset64;
> +        CHECK(compileAndRun<int64_t>(proc, address) == -42);
> +    }
> +    {
> +        Procedure proc;
> +        BasicBlock* root = proc.addBlock();
> +        int32_t x = -42;
> +        Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
> +        root->appendNew<ControlValue>(
> +            proc, Return, Origin(),
> +            root->appendNew<MemoryValue>(
> +                proc, Load, Int32, Origin(),
> +                base,
> +                offset32));
> +
> +        char* address = reinterpret_cast<char*>(&x) - offset32;
> +        CHECK(compileAndRun<int32_t>(proc, address) == -42);
> +    }
> +}

nit: the 64-bit and 32-bit sections are almost identical except for some type and variable differences.  Would it be better to templatize the common form and express these 2 sections as 2 instantiations of the template?  Something like:

template <typename T, B3::Type b3Type>
void testLoadWithOffsetImpl(int32_t offset)
{
       Procedure proc;
       BasicBlock* root = proc.addBlock();
       T x = -42;
       Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
       root->appendNew<ControlValue>(
           proc, Return, Origin(),
           root->appendNew<MemoryValue>(
               proc, Load, b3Type, Origin(),
               base,
               offset));

       char* address = reinterpret_cast<char*>(&x) - offset;
       CHECK(compileAndRun<T>(proc, address) == -42);
}

void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32)
{
    testLoadWithOffsetImpl<int64_t, Int64>(offset64);
    testLoadWithOffsetImpl<int32_t, Int32>(offset32);
}
Comment 4 Filip Pizlo 2016-01-13 09:30:34 PST
Comment on attachment 268870 [details]
Patch

Nice!
Comment 5 Filip Pizlo 2016-01-13 09:31:09 PST
R=me too
Comment 6 Filip Pizlo 2016-01-13 09:34:25 PST
(In reply to comment #3)
> Comment on attachment 268870 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268870&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        There are two available addressing: signed 9bits and unsigne scaled 12bits.
> 
> typo: "unsigne" ==> "unsigned".
> 
> > Source/JavaScriptCore/b3/B3LegalizeMemoryOffsets.h:36
> > +// compute it explicitely.
> 
> typo: "explicitely" ==> "explicitly".
> 
> > Source/JavaScriptCore/b3/testb3.cpp:171
> > +void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32)
> > +{
> > +    {
> > +        Procedure proc;
> > +        BasicBlock* root = proc.addBlock();
> > +        int64_t x = -42;
> > +        Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
> > +        root->appendNew<ControlValue>(
> > +            proc, Return, Origin(),
> > +            root->appendNew<MemoryValue>(
> > +                proc, Load, Int64, Origin(),
> > +                base,
> > +                offset64));
> > +
> > +        char* address = reinterpret_cast<char*>(&x) - offset64;
> > +        CHECK(compileAndRun<int64_t>(proc, address) == -42);
> > +    }
> > +    {
> > +        Procedure proc;
> > +        BasicBlock* root = proc.addBlock();
> > +        int32_t x = -42;
> > +        Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
> > +        root->appendNew<ControlValue>(
> > +            proc, Return, Origin(),
> > +            root->appendNew<MemoryValue>(
> > +                proc, Load, Int32, Origin(),
> > +                base,
> > +                offset32));
> > +
> > +        char* address = reinterpret_cast<char*>(&x) - offset32;
> > +        CHECK(compileAndRun<int32_t>(proc, address) == -42);
> > +    }
> > +}
> 
> nit: the 64-bit and 32-bit sections are almost identical except for some
> type and variable differences.  Would it be better to templatize the common
> form and express these 2 sections as 2 instantiations of the template? 
> Something like:
> 
> template <typename T, B3::Type b3Type>
> void testLoadWithOffsetImpl(int32_t offset)
> {
>        Procedure proc;
>        BasicBlock* root = proc.addBlock();
>        T x = -42;
>        Value* base = root->appendNew<ArgumentRegValue>(proc, Origin(),
> GPRInfo::argumentGPR0);
>        root->appendNew<ControlValue>(
>            proc, Return, Origin(),
>            root->appendNew<MemoryValue>(
>                proc, Load, b3Type, Origin(),
>                base,
>                offset));
> 
>        char* address = reinterpret_cast<char*>(&x) - offset;
>        CHECK(compileAndRun<T>(proc, address) == -42);
> }
> 
> void testLoadWithOffsetImpl(int32_t offset64, int32_t offset32)
> {
>     testLoadWithOffsetImpl<int64_t, Int64>(offset64);
>     testLoadWithOffsetImpl<int32_t, Int32>(offset32);
> }

For what it's worth we haven't been trying too hard to reduce code duplication in the tests. Reducing code duplication takes work, and it is most profitable when you intend to somehow evolve the code later - so you don't have to change the code in more than one place. But we don't really change test code after its written. Even if we made a B3 change that somehow made a test not relevant, we would probably write a new test and keep the old one for regression coverage. It's conceivable that we would change IR in a way that makes tests break or not compile, but then we would probably have to search-replace all of testb3 anyway. 

TL:DR; I'm not opposed to deduplicating code but it's not a priority for tests.
Comment 7 Benjamin Poulain 2016-01-13 10:25:59 PST
Created attachment 268875 [details]
Patch for landing
Comment 8 Mark Lam 2016-01-13 10:27:16 PST
Comment on attachment 268875 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGCommon.h:41
> -#define FTL_USES_B3 0
> +#define FTL_USES_B3 1

Is this intentional?
Comment 9 Benjamin Poulain 2016-01-13 10:31:00 PST
(In reply to comment #8)
> Comment on attachment 268875 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268875&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCommon.h:41
> > -#define FTL_USES_B3 0
> > +#define FTL_USES_B3 1
> 
> Is this intentional?

Shit! Good catch. I forgot the -g.
Comment 10 Benjamin Poulain 2016-01-13 10:33:09 PST
Created attachment 268876 [details]
Patch for landing
Comment 11 Benjamin Poulain 2016-01-13 10:38:50 PST
I kept the duplicated code. I find it easier to debug on device since the only thing we get at first is the faulty line number.
Comment 12 WebKit Commit Bot 2016-01-13 11:33:16 PST
Comment on attachment 268876 [details]
Patch for landing

Clearing flags on attachment: 268876

Committed r194969: <http://trac.webkit.org/changeset/194969>
Comment 13 WebKit Commit Bot 2016-01-13 11:33:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Ryan Haddad 2016-01-13 11:51:08 PST
This change appears to have broken the iOS build:
<https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/builds/1947>

Undefined symbols for architecture arm64:
  "__ZN3JSC2B35Value2asINS0_11MemoryValueEEEPT_v", referenced from:
      __ZN3JSC2B321legalizeMemoryOffsetsERNS0_9ProcedureE in B3LegalizeMemoryOffsets.o
Comment 15 Chris Dumez 2016-01-13 12:47:39 PST
(In reply to comment #14)
> This change appears to have broken the iOS build:
> <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/
> builds/1947>
> 
> Undefined symbols for architecture arm64:
>   "__ZN3JSC2B35Value2asINS0_11MemoryValueEEEPT_v", referenced from:
>       __ZN3JSC2B321legalizeMemoryOffsetsERNS0_9ProcedureE in
> B3LegalizeMemoryOffsets.o

Same here :(
Comment 16 WebKit Commit Bot 2016-01-13 12:51:56 PST
Re-opened since this is blocked by bug 153075
Comment 17 Benjamin Poulain 2016-01-15 15:40:37 PST
Created attachment 269111 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2016-01-15 16:37:42 PST
Comment on attachment 269111 [details]
Patch for landing

Clearing flags on attachment: 269111

Committed r195159: <http://trac.webkit.org/changeset/195159>
Comment 19 WebKit Commit Bot 2016-01-15 16:37:48 PST
All reviewed patches have been landed.  Closing bug.