Bug 153065 – [JSC] Legalize Memory Offsets for ARM64 before lowering to Air

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Legalize Memory Offsets for ARM64 before lowering to Air

[Benjamin Poulain]

Created attachment 268870 [details]
Patch

[WebKit Commit Bot]

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.

[Mark Lam]

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);
}

[Filip Pizlo]

Comment on attachment 268870 [details]
Patch

Nice!

[Filip Pizlo]

R=me too

[Filip Pizlo]

(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.

[Benjamin Poulain]

Created attachment 268875 [details]
Patch for landing

[Mark Lam]

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?

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 268876 [details]
Patch for landing

[Benjamin Poulain]

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.

[WebKit Commit Bot]

Comment on attachment 268876 [details]
Patch for landing

Clearing flags on attachment: 268876

Committed r194969: <http://trac.webkit.org/changeset/194969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Chris Dumez]

(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 :(

[WebKit Commit Bot]

Re-opened since this is blocked by bug 153075

[Benjamin Poulain]

Created attachment 269111 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 269111 [details]
Patch for landing

Clearing flags on attachment: 269111

Committed r195159: <http://trac.webkit.org/changeset/195159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.