Bug 151474 - [JSC] Add Air lowering to BitNot() for Xor(value, -1)
Summary: [JSC] Add Air lowering to BitNot() for Xor(value, -1)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-19 18:55 PST by Benjamin Poulain
Modified: 2015-11-20 14:07 PST (History)
7 users (show)

See Also:


Attachments
Patch (26.50 KB, patch)
2015-11-19 18:56 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (18.48 KB, patch)
2015-11-19 19:54 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (18.52 KB, patch)
2015-11-20 12:23 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (19.21 KB, patch)
2015-11-20 13:07 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-11-19 18:55:28 PST
[JSC] Add B3 BitNot and Strength Reduction
Comment 1 Benjamin Poulain 2015-11-19 18:56:32 PST
Created attachment 265927 [details]
Patch
Comment 2 Filip Pizlo 2015-11-19 19:18:11 PST
Comment on attachment 265927 [details]
Patch

We don't need a new opcode for this. BitNot(x) is just BitXor(x, -1). I don't think it's useful to have a BitNot instruction when BitXor with an immediate serves the same purpose. Also, it would be weird to have BitNot but not Neg - Neg is currently handled using Sub(0, x), sort of like how I'm proposing to handle Not using Xor.
Comment 3 Benjamin Poulain 2015-11-19 19:51:24 PST
(In reply to comment #2)
> Comment on attachment 265927 [details]
> Patch
> 
> We don't need a new opcode for this. BitNot(x) is just BitXor(x, -1). I
> don't think it's useful to have a BitNot instruction when BitXor with an
> immediate serves the same purpose. Also, it would be weird to have BitNot
> but not Neg - Neg is currently handled using Sub(0, x), sort of like how I'm
> proposing to handle Not using Xor.

Yep, that's a better idea.
Comment 4 Benjamin Poulain 2015-11-19 19:54:32 PST
Created attachment 265934 [details]
Patch
Comment 5 Filip Pizlo 2015-11-19 22:31:26 PST
Comment on attachment 265934 [details]
Patch

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

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1319
> +                appendUnOp<Not32, Not64, Air::Oops>(m_value->child(0));

this is missing a break statement. ;-)
Comment 6 Benjamin Poulain 2015-11-20 12:23:52 PST
Created attachment 265979 [details]
Patch
Comment 7 Filip Pizlo 2015-11-20 12:44:54 PST
Comment on attachment 265979 [details]
Patch

Don't forget the Store(Not(Load)) case.
Comment 8 Benjamin Poulain 2015-11-20 13:07:38 PST
Created attachment 265981 [details]
Patch
Comment 9 Benjamin Poulain 2015-11-20 14:07:08 PST
Comment on attachment 265981 [details]
Patch

Clearing flags on attachment: 265981

Committed r192696: <http://trac.webkit.org/changeset/192696>
Comment 10 Benjamin Poulain 2015-11-20 14:07:12 PST
All reviewed patches have been landed.  Closing bug.