Bug 150720 - B3 should be able to compile a control flow diamond
Summary: B3 should be able to compile a control flow diamond
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 150725 150279 150721 150726 150727
  Show dependency treegraph
 
Reported: 2015-10-30 10:42 PDT by Filip Pizlo
Modified: 2015-10-30 14:50 PDT (History)
11 users (show)

See Also:


Attachments
work in progress (32.83 KB, patch)
2015-10-30 12:43 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (49.16 KB, patch)
2015-10-30 13:48 PDT, Filip Pizlo
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-10-30 10:42:33 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2015-10-30 12:43:22 PDT
Created attachment 264411 [details]
work in progress
Comment 2 Filip Pizlo 2015-10-30 13:48:43 PDT
Created attachment 264413 [details]
the patch
Comment 3 WebKit Commit Bot 2015-10-30 13:50:25 PDT
Attachment 264413 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:540:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:541:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:541:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:573:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:574:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:574:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:603:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:604:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:604:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:67:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCode.cpp:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.cpp:146:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.cpp:178:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:47:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:48:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 15 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Benjamin Poulain 2015-10-30 14:40:51 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=264413&action=review

> Source/JavaScriptCore/b3/B3Const64Value.cpp:68
> +    return proc.add<Const64Value>(origin(), m_value == other->asInt64());

Oops.

> Source/JavaScriptCore/b3/B3Const64Value.cpp:75
> +    return proc.add<Const64Value>(origin(), m_value != other->asInt64());

ditto

> Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:68
> +    return proc.add<ConstDoubleValue>(origin(), m_value == other->asDouble());

ditto

> Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:75
> +    return proc.add<ConstDoubleValue>(origin(), m_value != other->asDouble());

ditto

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:683
> +        // In B3 it's possible to branch on any value. The semantics of:
> +        //
> +        //     Branch(value)
> +        //
> +        // Are guaranteed to be identical to:
> +        //
> +        //     Branch(NotEqual(value, 0))

I have the feeling this comment would have more visibility next to the opcode definition instead of at the lowering phase.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:253
> +                m_changed = true;
> +                m_changedCFG = true;

Might be easier to loop over (m_changed || m_changedCFG) since m_changedCFG implies m_changed

> Source/JavaScriptCore/b3/B3UpsilonValue.h:42
> +    void setPhi(Value* phi) { m_phi = phi; }

ASSERT(phi->type() == [value]->type());
ASSERT(phi->opcode() == phi)

> Source/JavaScriptCore/b3/B3UpsilonValue.h:57
> +        if (phi)

ASSERT(phi->opcode() == phi)
Comment 5 Benjamin Poulain 2015-10-30 14:42:01 PDT
Comment on attachment 264413 [details]
the patch

Please check the previous comments before landing.
Comment 6 Filip Pizlo 2015-10-30 14:43:02 PDT
(In reply to comment #4)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264413&action=review
> 
> > Source/JavaScriptCore/b3/B3Const64Value.cpp:68
> > +    return proc.add<Const64Value>(origin(), m_value == other->asInt64());
> 
> Oops.
> 
> > Source/JavaScriptCore/b3/B3Const64Value.cpp:75
> > +    return proc.add<Const64Value>(origin(), m_value != other->asInt64());
> 
> ditto
> 
> > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:68
> > +    return proc.add<ConstDoubleValue>(origin(), m_value == other->asDouble());
> 
> ditto
> 
> > Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:75
> > +    return proc.add<ConstDoubleValue>(origin(), m_value != other->asDouble());
> 
> ditto

Fixed those.

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:683
> > +        // In B3 it's possible to branch on any value. The semantics of:
> > +        //
> > +        //     Branch(value)
> > +        //
> > +        // Are guaranteed to be identical to:
> > +        //
> > +        //     Branch(NotEqual(value, 0))
> 
> I have the feeling this comment would have more visibility next to the
> opcode definition instead of at the lowering phase.

B3Opcode.h has a sort of similar comment.

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:253
> > +                m_changed = true;
> > +                m_changedCFG = true;
> 
> Might be easier to loop over (m_changed || m_changedCFG) since m_changedCFG
> implies m_changed

I changed it so that the "if (m_changedCFG)" check in the fixpoint also sets m_changed, so if you set m_changedCFG it implies m_changed automatically.

> 
> > Source/JavaScriptCore/b3/B3UpsilonValue.h:42
> > +    void setPhi(Value* phi) { m_phi = phi; }
> 
> ASSERT(phi->type() == [value]->type());
> ASSERT(phi->opcode() == phi)
> 
> > Source/JavaScriptCore/b3/B3UpsilonValue.h:57
> > +        if (phi)
> 
> ASSERT(phi->opcode() == phi)

Fixed.
Comment 7 Filip Pizlo 2015-10-30 14:50:27 PDT
Landed in http://trac.webkit.org/changeset/191816