Bug 152231 – [JSC] Add ceil() support for x86 and expose it to B3

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Add ceil() support for x86 and expose it to B3

[Benjamin Poulain]

Created attachment 267270 [details]
Patch

[Benjamin Poulain]

Created attachment 267273 [details]
Patch

[Geoffrey Garen]

Comment on attachment 267273 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 267273 [details]
Patch

Clearing flags on attachment: 267273

Committed r194062: <http://trac.webkit.org/changeset/194062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Csaba Osztrogonác]

(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