[Webkit-unassigned] [Bug 235581] Add support for WASM branch hinting proposal

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 25 10:39:27 PST 2022


https://bugs.webkit.org/show_bug.cgi?id=235581

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #449921|review?                     |review-
              Flags|                            |

--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 449921
  --> https://bugs.webkit.org/attachment.cgi?id=449921
Patch

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

Your implementation in B3/Air is more complicated than it needs to be. All you need to do is set the FrequencyClass based on the hint. If the hint is Likely, set the taken side  "Normal" and not taken side "Rare". If the hint is Unlikely, set it the taken side to "Rare" and the not taken sideT "Normal". That'll get you the behavior you want. No need to change static execution count estimate, no need to add the "isHinted" stuff to any of the basic block classes.

> Source/JavaScriptCore/b3/B3BasicBlock.h:150
> +    void setHinted(bool hinted) { m_hinted = hinted; }

none of this is necessary. See my suggestion on how to implement this patch.

> Source/JavaScriptCore/b3/air/AirBasicBlock.h:137
> +    void setHinted(bool hinted) { m_hinted = hinted; }
> +    bool isHinted() const { return m_hinted; }

none of this is necessary. See my suggestion on how to implement this patch.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2498
> +auto B3IRGenerator::addIfWithHint(ExpressionType condition, BlockSignature signature, Stack& enclosingStack, ControlType& result, Stack& newStack, const BranchHint& hint) -> PartialResult

ditto to what I wrote about the AirIRGenerator having two versions of this function.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2755
> +    if (Options::useWebAssemblyBranchHints()) {
> +        const BranchHint& hint = m_info.getBranchHint(m_functionIndex, m_parser->currentOpcodeStartingOffset());
> +        if (hint != BranchHint::Invalid)
> +            return addBranchWithHint(data, condition, returnValues, hint);
> +    }

Instead of the second "addBranchWithHint" function, just define default values for FrequencyClass for taken/not taken. Then if we find a branch hint inside the map, you can change the values of those frequency classes.

Also, no need for "const BranchHint&". Just "BranchHint hint = m_info.get(...);"

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2775
> +auto B3IRGenerator::addBranchWithHint(ControlData& data, ExpressionType condition, const Stack& returnValues, const BranchHint& hint) -> PartialResult

You shouldn't use two versions of essentially an identical function. Just have default values for the FrequencyClass used and augment them when we have hint data.

> Source/JavaScriptCore/wasm/WasmBranchHints.h:59
> +    StdUnorderedMap<uint32_t, BranchHint> m_map;

you should use HashMap here (and with UnsignedWithZeroKeyHashTraits  if the branch offset can be zero)

> Source/JavaScriptCore/wasm/WasmBranchHints.h:62
> +

all functions below here can be constexpr.

Also, no need to really take in this enum as "const BranchHint&"

> Source/JavaScriptCore/wasm/WasmBranchHints.h:70
> +inline BranchHint branchHintToInverse(const BranchHint& hint)

branchHintToInverse => "invertBranchHint"

> Source/JavaScriptCore/wasm/WasmBranchHints.h:84
> +        break;

maybe just return false here?

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:39
> +    int64_t lastFunctionIndex = -1;

name nit: "lastFunctionIndex" => "previousFunctionIndex"

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:46
> +        WASM_PARSER_FAIL_IF(int64_t(functionIndex) <= lastFunctionIndex, "invalid function index ", functionIndex, " for function ", i);

webkit style guide uses static_cast<int64_t>, not C style casts.

Also, is this really "<="? Why not strictly "<"? The spec says we can keep having functions of the same index? That has consequences on your algorithm below using "emplace"

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:60
> +            WASM_PARSER_FAIL_IF(int64_t(branchOffset) <= lastBranchOffset, "invalid branch offset ", branchOffset, " for hint ", j);

same nit regarding c style casts


Also, when using HashMap, we should fail if the branchOffset is either the empty or deleted value in the hash map. See the hash trait I was talking about:

template<typename T> struct UnsignedWithZeroKeyHashTraits : GenericHashTraits<T> {
    static constexpr bool emptyValueIsZero = false;
    static T emptyValue() { return std::numeric_limits<T>::max(); }
    static void constructDeletedValue(T& slot) { slot = std::numeric_limits<T>::max() - 1; }
    static bool isDeletedValue(T value) { return value == std::numeric_limits<T>::max() - 1; }
};


Which in general should be fine. Nobody should be using such a large offset anyways.

Also, same question regarding "<=" instead of "<"

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:71
> +            BranchHint branchHint = static_cast<BranchHint>(int32_t(parsedBranchHint));

don't use C style cast here. But also, I don't see why it's necessary at all. Shouldn't static_cast<BranchHint>(parsedBranchHint) work?

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:72
> +            WASM_PARSER_FAIL_IF(!isValidBranchHint(branchHint), "invalid branch hint value ", parsedBranchHint, " for hint ", j);

how is this possible if we're parsing a VarUint1? The value has to be zero or one, right? Should this be an assert?

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:74
> +            branchHintsForFunction.insert(branchOffset, branchHint);

related question regarding the "<=" above. If functionIndex already exists, what semantics do we want? Merge? Overwrite? Skip?

> Source/JavaScriptCore/wasm/WasmBranchHintsSectionParser.cpp:76
> +        m_info->branchHints.emplace(functionIndex, WTFMove(branchHintsForFunction));

related question regarding the "<=" above. If functionIndex already exists, what semantics do we want? Merge? Overwrite? Skip?

> Source/JavaScriptCore/wasm/WasmModuleInformation.h:40
> +    using BranchHints = StdUnorderedMap<uint32_t, BranchHintMap>;

you should use HashMap here with UnsignedWithZeroKeyHashTraits

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220125/0844c46d/attachment-0001.htm>


More information about the webkit-unassigned mailing list