Bug 151124

Summary:

B3 should be able to compile and canonicalize Mul

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, commit-queue, ggaren, mark.lam, mhahnenb, msaboff, nrotem, oliver, saam, sam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

150279

Attachments:

Description	Flags
work in progress	none
a bit more	none
the patch	ggaren: review+

Description Filip Pizlo 2015-11-10 18:58:03 PST

Patch forthcoming.

Comment 1 Filip Pizlo 2015-11-10 18:59:23 PST

Created attachment 265260 [details]
work in progress

Comment 2 Filip Pizlo 2015-11-10 19:47:01 PST

Created attachment 265261 [details]
a bit more

Comment 3 Filip Pizlo 2015-11-10 21:20:41 PST

Created attachment 265268 [details]
the patch

Comment 4 WebKit Commit Bot 2015-11-10 21:23:42 PST

Attachment 265268 [details] did not pass style-queue:


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/testb3.cpp:209:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:302:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2855:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:2861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 5 Geoffrey Garen 2015-11-10 21:32:59 PST

Comment on attachment 265268 [details]
the patch

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

r=me

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:227
> +                // Turn this: Mul(value, 1)
> +                // Into this: value
> +                if (factor == 1) {
> +                    m_value->replaceWithIdentity(m_value->child(0));
> +                    break;
> +                }

Should we do -1 here?

Comment 6 Filip Pizlo 2015-11-10 21:40:54 PST

(In reply to comment #5)
> Comment on attachment 265268 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265268&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:227
> > +                // Turn this: Mul(value, 1)
> > +                // Into this: value
> > +                if (factor == 1) {
> > +                    m_value->replaceWithIdentity(m_value->child(0));
> > +                    break;
> > +                }
> 
> Should we do -1 here?

Good point!  We should.

Comment 7 Filip Pizlo 2015-11-11 12:35:49 PST

Landed in http://trac.webkit.org/changeset/192320