Summary: | [JSC] Add Float support to B3 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, fpizlo | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Benjamin Poulain
2015-12-07 17:12:40 PST
Created attachment 266835 [details]
Patch
*** Bug 151921 has been marked as a duplicate of this bug. *** *** Bug 151903 has been marked as a duplicate of this bug. *** Attachment 266835 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382: More than one command on the same line [whitespace/newline] [4]
Total errors found: 13 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 266836 [details]
Patch
Attachment 266836 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382: More than one command on the same line [whitespace/newline] [4]
Total errors found: 13 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 266837 [details]
Patch
Attachment 266837 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382: More than one command on the same line [whitespace/newline] [4]
Total errors found: 13 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 266837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266837&action=review I think you're just missing moveFloatConditionally, which corresponds to Float = Select(Int, Float, Float). But I guess that depends on whether or not you believe that movsd can efficiently be used instead of movss. > Source/JavaScriptCore/ChangeLog:8 > + This patch adds basic float support to B3. Basic or comprehensive? > Source/JavaScriptCore/assembler/MacroAssembler.h:1365 > + > + void moveDoubleConditionallyFloat(DoubleCondition cond, FPRegisterID left, FPRegisterID right, FPRegisterID src, FPRegisterID dest) > + { > + Jump falseCase = branchFloat(invert(cond), left, right); > + moveDouble(src, dest); > + falseCase.link(this); > + } What about moveFloatConditionally? > Source/JavaScriptCore/b3/B3Generate.cpp:78 > + reduceDoubleToFloat(procedure); > + Any particular reason why this isn't run after reduceStrength()? Actually, why isn't this just part of reduceStrength()? > Source/JavaScriptCore/b3/B3LowerToAir.cpp:783 > + case Float: This doesn't seem right. Is it really optimal to use movsd when we just need movss? Comment on attachment 266837 [details]
Patch
We talked about the moveFloat/moveFloatConditionally issue offline.
Comment on attachment 266837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266837&action=review > Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:85 > + HashSet<Value*> candidates; Why not IndexSet<Value> candidates? Committed r193683: <http://trac.webkit.org/changeset/193683> Comment on attachment 266837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266837&action=review > Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:72 > +// Such operation arise in JS because there are is no Float type are is > Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:76 > +// Using float in place remove the useless conversion, and the float removes > Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:77 > +// ops is sometime massively cheaper (SQRT for example). op |