[webkit-reviews] review denied: [Bug 196578] B3 peephole optimizations should live in their own file, and be used by FTLOutput.cpp : [Attachment 367692] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 196578: B3 peephole optimizations should live in their own file, and be
used by FTLOutput.cpp
https://bugs.webkit.org/show_bug.cgi?id=196578

Attachment 367692: Patch

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




--- Comment #7 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 367692
  --> https://bugs.webkit.org/attachment.cgi?id=367692
Patch

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

On the one hand, I like this change.  But on the other hand, it doesn't seem
like it's a measurable improvement and it means that adding new
strength-reduction rules may be annoying since for some opcodes you need to
have a case in reduceStrength and a function on the PeepholeOptimizer.

I think that we should try to have the simplest compiler we can get away with
while getting great perf.  So, that means not taking this change.

On the other hand, if using this change in conjunction with some other change
does result in a measurable improvement, then we should take it.

So, that means not taking the change until there is another change that makes
this one valuable.

> Source/JavaScriptCore/b3/B3LowerMacros.cpp:158
> +		       Value* divResult = peephole.reduceDiv(m_value->type(),
m_origin, m_value->child(0), m_value->child(1), true);

WebKit style says you should never pass the literal true or false as an
argument to a function except if the name of the function makes it obvious what
the meaning of that boolean is.

There are two accepted ways of side-stepping this issue:

1) Always name the argument at the callsite:

    bool shouldPropagateType = true;
    thingy->fooBar(stuff, shouldPropagateType);

2) Replace with an enum:

    enum ShouldPropagateTypeMode { ShouldPropagateType, ShouldNotPropagateType
};
    thingy->fooBar(stuff, ShouldPropagateType);

The second one is preferred.