Bug 152231 - [JSC] Add ceil() support for x86 and expose it to B3
Summary: [JSC] Add ceil() support for x86 and expose it to B3
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-12-13 14:05 PST by Benjamin Poulain
Modified: 2015-12-15 01:14 PST (History)
7 users (show)

See Also:


Attachments
Patch (36.98 KB, patch)
2015-12-13 14:07 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (38.06 KB, patch)
2015-12-13 16:19 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-12-13 14:05:43 PST
[JSC] Add ceil() support for x86 and expose it to B3
Comment 1 Benjamin Poulain 2015-12-13 14:07:47 PST
Created attachment 267270 [details]
Patch
Comment 2 Benjamin Poulain 2015-12-13 16:19:38 PST
Created attachment 267273 [details]
Patch
Comment 3 Geoffrey Garen 2015-12-14 13:11:07 PST
Comment on attachment 267273 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2015-12-14 14:44:48 PST
Comment on attachment 267273 [details]
Patch

Clearing flags on attachment: 267273

Committed r194062: <http://trac.webkit.org/changeset/194062>
Comment 5 WebKit Commit Bot 2015-12-14 14:44:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2015-12-14 14:49:55 PST
Comment on attachment 267273 [details]
Patch

I think it's weird that this is implemented as a B3 opcode.  This is the kind of thing that we could do with a Patchpoint.  The benefit of a Patchpoint is:

- You don't need to add an Air opcode where isValidForm() returns true even when the opcode is not allowed.
- You don't need to add so much code.

I'm really concerned with Inst::isValidForm() being wrong after your patch lands.  If you want to add Ceil to Air, then I think that this patch should be blocked on making opcode_generator.rb understand the notion of conditional instructions.
Comment 7 Filip Pizlo 2015-12-14 15:13:58 PST
We should take a chill pill on putting new opcodes into B3.  There are too many opcodes for very specific math things.  This isn't scalable.  If we allowed everyone to add their favorite X86 opcodes as B3 opcodes, we'd have an unmaintainable compiler.  It's very important to punt some subset of target instructions, even if it means that there exist hypothetical optimizations opportunities that we possibly overlook because of it.

The way that LLVM approached ceil is that it's an intrinsic.  B3's answer to intrinsics is Patchpoint.  We should probably have used Patchpoint here.  If at some time in the future we find that we need to also support algebraic optimizations on Ceil (or whatever fancy thing someone comes up with), then we can think about what it would look like to make Patchpoint reveal enough about its semantics that ReduceStrength could do things to it.  Or, if we find a benchmark where Ceil-as-Patchpoint is a performance problem then we could embark on a project to give it its own B3 opcode.  But I suspect that we won't need any of that for a long time, and that all that the users of Ceil want is things that you could get from a Patchpoint.
Comment 8 Csaba Osztrogonác 2015-12-15 01:14:42 PST
(In reply to comment #4)
> Comment on attachment 267273 [details]
> Patch
> 
> Clearing flags on attachment: 267273
> 
> Committed r194062: <http://trac.webkit.org/changeset/194062>

Just to document:
 - a fix landed in https://trac.webkit.org/changeset/194071
 - an ARM buildfix landed in https://trac.webkit.org/changeset/194101