[webkit-reviews] review denied: [Bug 235581] Add support for WASM branch hinting proposal : [Attachment 449921] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied tom at leaningtech.com's request for
review:
Bug 235581: Add support for WASM branch hinting proposal
https://bugs.webkit.org/show_bug.cgi?id=235581

Attachment 449921: Patch

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




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