Created attachment 267281 [details] the patch

Comment on attachment 267281 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=267281&action=review Does it mean that you are planning to support 32 bit architectures too? (x86 and ARMv7) I just ask, because these platforms aren't supported by FTL JIT now. > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:96 > +# armv6: means just armv7. typo: not armv6, but armv7

Comment on attachment 267281 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=267281&action=review Nicely done. > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:78 > +# x86_64: Fuzz UD:G, D:G > +# Tmp, Tmp > +# arm64: Tmp, Addr I am not sure what this does since arm64 is not a subset of x86_64. Did you mean to have the "x86_64" for the instruction? > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:100 > It might be useful to also have an AVX runtime check in the future. It looks like all the floating point ops have a nicer AVX version on X86.

(In reply to comment #3) > Comment on attachment 267281 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267281&action=review > > Nicely done. > > > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:78 > > +# x86_64: Fuzz UD:G, D:G > > +# Tmp, Tmp > > +# arm64: Tmp, Addr > > I am not sure what this does since arm64 is not a subset of x86_64. > > Did you mean to have the "x86_64" for the instruction? I meant for this example to say: Fuzz UD:G D:G Tmp, Tmp arm64: Tmp, Addr > > > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:100 > > > > It might be useful to also have an AVX runtime check in the future. > > It looks like all the floating point ops have a nicer AVX version on X86. Long term, I think it's good to do that. But using AVX introduces hazards when you mix with SSE, and the rest of JSC's JITs use SSE a lot. We had to disable AVX in FTL to prevent horrendous 2x regressions. Also, AVX instructions have a super fat encoding. I'm not sure that the benefit we get from three-operand form is enough to outweigh the reduced instruction density. So, let's put off AVX until after we are happy with our SSE-based stuff. Final thought: currently, the architecture selection in AirOpcode.opcodes is compile-time, since the generator emits #if's. We would have to add the ability to do run-time checks if we wanted to have AVX. It would probably not be practical for Inst::isValidForm() to use 'cpuid' everytime it's called to check if AVX is present, so then we'll need some machinery to cache all of our 'cpuid' queries. Seems like a lot of work. :-)

(In reply to comment #2) > Comment on attachment 267281 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267281&action=review > > Does it mean that you are planning to support 32 bit architectures too? (x86 > and ARMv7) Sort of. B3 doesn't support 32-bit but it has a lot of the scaffolding needed to support it. > I just ask, because these platforms aren't supported by FTL JIT now. Getting FTL to support 32-bit would be a lot of work. Getting B3 to support 32-bit would require just: 1) Fix some minor bugs where code statically assumes 64-bit. We have a few of those, especially in the test suite. 2) Add a 64-bit-to-32-bit lowering for Int64's. One way to do it is to do the lowering in Air right after instruction selection, since you need to lower to 32-bit operations that produce multiple results and store some of their results in flags. There's no way to express "add with carry" in B3 currently. We aren't planning on doing this anytime soon, but I personally want to leave the door open. I suspect that B3 will be useful to non-JSC clients. For example, we might want to use it for YARR, or whatever other JIT someone comes up with in the future. So, B3 will probably support more architectures than FTL does, because B3 isn't meant to be owned exclusively by FTL. > > > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:96 > > +# armv6: means just armv7. > > typo: not armv6, but armv7 Fixed, thanks!

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