Changeset 248710 in webkit


Ignore:
Timestamp:
Aug 14, 2019 11:39:28 PM (5 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] Air does not appropriately propagate ConstFloatValue to stackmap
​https://bugs.webkit.org/show_bug.cgi?id=200759

Reviewed by Saam Barati.

In B3MoveConstant phase, we convert ConstFloatValue and ConstDoubleValue to memory access to the table
to avoid large immediates *except for* stackmap argument case. This is because materializing constant doubles
and floats as memory-access before passing it to stackmap is wasteful: the stackmap may not use it actually, or
stackmap can do better job if it knows the parameter is constant.

Based on the above operation, B3LowerToAir phase strongly assumes that all ConstFloatValue and ConstDoubleValue
are removed except for the case used for parameter of stackmap. With r192377, B3LowerToAir catch this case, and
propagate constant double value as ValueRep in stackmap. While B3LowerToAir does this correctly for ConstDoubleValue,
we missed adding this support for ConstFloatValue.

This patch adds r192377's support for ConstFloatValue to propagate ConstFloatValue correctly to the stackmap.
This issue starts appearing since Wasm BBQ-B3 OSR starts putting ConstFloatValue to OSR-tier-up patchpoint.

  • b3/B3LowerToAir.cpp:
  • b3/B3ValueKey.h:

(JSC::B3::ValueKey::ValueKey):
(JSC::B3::ValueKey::floatValue const):

  • b3/B3ValueRep.h:

(JSC::B3::ValueRep::constantFloat):
(JSC::B3::ValueRep::floatValue const):

  • b3/testb3.h:
  • b3/testb3_1.cpp:

(run):

  • b3/testb3_5.cpp:

(testPatchpointManyWarmAnyImms):
(testPatchpointManyColdAnyImms):
(testPatchpointManyImms): Deleted.

Location:
trunk/Source/JavaScriptCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r248709 r248710  
     12019-08-14  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Air does not appropriately propagate ConstFloatValue to stackmap
     4        https://bugs.webkit.org/show_bug.cgi?id=200759
     5
     6        Reviewed by Saam Barati.
     7
     8        In B3MoveConstant phase, we convert ConstFloatValue and ConstDoubleValue to memory access to the table
     9        to avoid large immediates *except for* stackmap argument case. This is because materializing constant doubles
     10        and floats as memory-access before passing it to stackmap is wasteful: the stackmap may not use it actually, or
     11        stackmap can do better job if it knows the parameter is constant.
     12
     13        Based on the above operation, B3LowerToAir phase strongly assumes that all ConstFloatValue and ConstDoubleValue
     14        are removed except for the case used for parameter of stackmap. With r192377, B3LowerToAir catch this case, and
     15        propagate constant double value as ValueRep in stackmap. While B3LowerToAir does this correctly for ConstDoubleValue,
     16        we missed adding this support for ConstFloatValue.
     17
     18        This patch adds r192377's support for ConstFloatValue to propagate ConstFloatValue correctly to the stackmap.
     19        This issue starts appearing since Wasm BBQ-B3 OSR starts putting ConstFloatValue to OSR-tier-up patchpoint.
     20
     21        * b3/B3LowerToAir.cpp:
     22        * b3/B3ValueKey.h:
     23        (JSC::B3::ValueKey::ValueKey):
     24        (JSC::B3::ValueKey::floatValue const):
     25        * b3/B3ValueRep.h:
     26        (JSC::B3::ValueRep::constantFloat):
     27        (JSC::B3::ValueRep::floatValue const):
     28        * b3/testb3.h:
     29        * b3/testb3_1.cpp:
     30        (run):
     31        * b3/testb3_5.cpp:
     32        (testPatchpointManyWarmAnyImms):
     33        (testPatchpointManyColdAnyImms):
     34        (testPatchpointManyImms): Deleted.
     35
    1362019-08-14  Keith Rollin  <krollin@apple.com>
    237

  • trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp

    r248178 r248710  
    13391339                    commitInternal(value.value());
    13401340                    arg = Arg::bigImm(bitwise_cast<int64_t>(value.value()->asDouble()));
     1341                } else if (value.value()->hasFloat() && canBeInternal(value.value())) {
     1342                    commitInternal(value.value());
     1343                    arg = Arg::bigImm(static_cast<uint64_t>(bitwise_cast<uint32_t>(value.value()->asFloat())));
    13411344                } else
    13421345                    arg = tmp(value.value());

  • trunk/Source/JavaScriptCore/b3/B3ValueKey.h

    r248178 r248710  
    8181        , m_type(type)
    8282    {
     83        // This means that upper 32bit of u.value is 0.
    8384        u.floatValue = value;
    8485    }
    … …  
    9394    int64_t value() const { return u.value; }
    9495    double doubleValue() const { return u.doubleValue; }
    95     double floatValue() const { return u.floatValue; }
     96    float floatValue() const { return u.floatValue; }
    9697
    9798    bool operator==(const ValueKey& other) const

  • trunk/Source/JavaScriptCore/b3/B3ValueRep.h

    r248178 r248710  
    160160    }
    161161
     162    static ValueRep constantFloat(float value)
     163    {
     164        return ValueRep::constant(static_cast<uint64_t>(bitwise_cast<uint32_t>(value)));
     165    }
     166
    162167    Kind kind() const { return m_kind; }
    163168
    … …  
    233238    }
    234239
     240    float floatValue() const
     241    {
     242        return bitwise_cast<float>(static_cast<uint32_t>(static_cast<uint64_t>(value())));
     243    }
     244
    235245    ValueRep withOffset(intptr_t offset) const
    236246    {

  • trunk/Source/JavaScriptCore/b3/testb3.h

    r248178 r248710  
    702702void testPatchpointLotsOfLateAnys();
    703703void testPatchpointAnyImm(ValueRep rep);
    704 void testPatchpointManyImms();
     704void testPatchpointManyWarmAnyImms();
     705void testPatchpointManyColdAnyImms();
    705706void testPatchpointWithRegisterResult();
    706707void testPatchpointWithStackArgumentResult();

  • trunk/Source/JavaScriptCore/b3/testb3_1.cpp

    r248533 r248710  
    476476    RUN(testPatchpointAnyImm(ValueRep::ColdAny));
    477477    RUN(testPatchpointAnyImm(ValueRep::LateColdAny));
    478     RUN(testPatchpointManyImms());
     478    RUN(testPatchpointManyWarmAnyImms());
     479    RUN(testPatchpointManyColdAnyImms());
    479480    RUN(testPatchpointWithRegisterResult());
    480481    RUN(testPatchpointWithStackArgumentResult());

  • trunk/Source/JavaScriptCore/b3/testb3_5.cpp

    r248178 r248710  
    2929#if ENABLE(B3_JIT)
    3030
    31 void testPatchpointManyImms()
     31void testPatchpointManyWarmAnyImms()
    3232{
    3333    Procedure proc;
    … …  
    3737    Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
    3838    Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
     39    Value* arg5 = root->appendNew<ConstFloatValue>(proc, Origin(), -42.5f);
    3940    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
    4041    patchpoint->append(ConstrainedValue(arg1, ValueRep::WarmAny));
    … …  
    4243    patchpoint->append(ConstrainedValue(arg3, ValueRep::WarmAny));
    4344    patchpoint->append(ConstrainedValue(arg4, ValueRep::WarmAny));
     45    patchpoint->append(ConstrainedValue(arg5, ValueRep::WarmAny));
    4446    patchpoint->setGenerator(
    4547        [&] (CCallHelpers&, const StackmapGenerationParams& params) {
    46             CHECK(params.size() == 4);
     48            CHECK(params.size() == 5);
    4749            CHECK(params[0] == ValueRep::constant(42));
    4850            CHECK(params[1] == ValueRep::constant(43));
    4951            CHECK(params[2] == ValueRep::constant(43000000000000ll));
    50             CHECK(params[3] == ValueRep::constant(bitwise_cast<int64_t>(42.5)));
     52            CHECK(params[3] == ValueRep::constantDouble(42.5));
     53            CHECK(params[4] == ValueRep::constantFloat(-42.5f));
     54            CHECK(params[4].floatValue() == -42.5f);
     55        });
     56    root->appendNewControlValue(
     57        proc, Return, Origin(),
     58        root->appendNew<Const32Value>(proc, Origin(), 0));
     59
     60    CHECK(!compileAndRun<int>(proc));
     61}
     62
     63void testPatchpointManyColdAnyImms()
     64{
     65    Procedure proc;
     66    BasicBlock* root = proc.addBlock();
     67    Value* arg1 = root->appendNew<Const32Value>(proc, Origin(), 42);
     68    Value* arg2 = root->appendNew<Const64Value>(proc, Origin(), 43);
     69    Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
     70    Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
     71    Value* arg5 = root->appendNew<ConstFloatValue>(proc, Origin(), -42.5f);
     72    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
     73    patchpoint->append(ConstrainedValue(arg1, ValueRep::ColdAny));
     74    patchpoint->append(ConstrainedValue(arg2, ValueRep::ColdAny));
     75    patchpoint->append(ConstrainedValue(arg3, ValueRep::ColdAny));
     76    patchpoint->append(ConstrainedValue(arg4, ValueRep::ColdAny));
     77    patchpoint->append(ConstrainedValue(arg5, ValueRep::ColdAny));
     78    patchpoint->setGenerator(
     79        [&] (CCallHelpers&, const StackmapGenerationParams& params) {
     80            CHECK(params.size() == 5);
     81            CHECK(params[0] == ValueRep::constant(42));
     82            CHECK(params[1] == ValueRep::constant(43));
     83            CHECK(params[2] == ValueRep::constant(43000000000000ll));
     84            CHECK(params[3] == ValueRep::constantDouble(42.5));
     85            CHECK(params[4] == ValueRep::constantFloat(-42.5f));
     86            CHECK(params[4].floatValue() == -42.5f);
    5187        });
    5288    root->appendNewControlValue(
Note: See TracChangeset for help on using the changeset viewer.