Bug 152886

Summary: [mips] Add countLeadingZeros32 implementation in macro assembler
Product: WebKit Reporter: Julien Brianceau <jbriance>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, commit-queue, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Add countLeadingZeros32 implementation in mips macro assembler
none
Add countLeadingZeros32 implementation in mips macro assembler (v2) msaboff: review+

Description Julien Brianceau 2016-01-08 05:05:36 PST
void countLeadingZeros32(RegisterID src, RegisterID dest) implementation is missing from mips macro assembler
Comment 1 Julien Brianceau 2016-01-08 05:19:37 PST
Created attachment 268538 [details]
Add countLeadingZeros32 implementation in mips macro assembler
Comment 2 Konstantin Tokarev 2016-01-08 05:26:54 PST
Comment on attachment 268538 [details]
Add countLeadingZeros32 implementation in mips macro assembler

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:318
> +        m_assembler.clz(dest, src);

Shouldn't we have a fallback here in case assembler does not support clz?
Comment 3 Julien Brianceau 2016-01-08 05:39:39 PST
(In reply to comment #2)
> Comment on attachment 268538 [details]
> Add countLeadingZeros32 implementation in mips macro assembler
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268538&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:318
> > +        m_assembler.clz(dest, src);
> 
> Shouldn't we have a fallback here in case assembler does not support clz?

It would be great to provide a generic fallback implementation indeed. However, mips32 is already a quite old ISA, so I doubt this would be useful to anyone.
Comment 4 Alex Christensen 2016-01-08 17:31:34 PST
Comment on attachment 268538 [details]
Add countLeadingZeros32 implementation in mips macro assembler

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

> Source/JavaScriptCore/assembler/MIPSAssembler.h:249
> +        RELEASE_ASSERT_NOT_REACHED();

I think this should be static_assert(false, "CLZ opcode is not available for this ISA"); so that someone will notice this when compiling instead of waiting to compile and do somewhat thorough testing on a device.
Comment 5 Julien Brianceau 2016-01-09 00:51:44 PST
Created attachment 268617 [details]
Add countLeadingZeros32 implementation in mips macro assembler (v2)
Comment 6 Julien Brianceau 2016-01-09 00:55:33 PST
(In reply to comment #4)
> 
> I think this should be static_assert(false, "CLZ opcode is not available for
> this ISA"); so that someone will notice this when compiling instead of
> waiting to compile and do somewhat thorough testing on a device.

You're right. I also took the opportunity to move the check from MIPSAssembler.h to MacroAssemblerMIPS.h, because if someone wants to write a generic implementation for an older ISA, MacroAssemblerMIPS.h is a better place than MIPSAssembler.h
Comment 7 Konstantin Tokarev 2016-01-09 05:27:58 PST
LGTM
Comment 8 Konstantin Tokarev 2016-01-14 13:54:25 PST
Could anyone approve this patch?
Comment 9 Michael Saboff 2016-01-14 14:24:50 PST
Comment on attachment 268617 [details]
Add countLeadingZeros32 implementation in mips macro assembler (v2)

Had to look up the encoding.
r=me
Comment 10 Julien Brianceau 2016-01-15 00:13:15 PST
Manually committed r195093: http://trac.webkit.org/changeset/195093