Bug 152886 – [mips] Add countLeadingZeros32 implementation in macro assembler

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Julien Brianceau]

void countLeadingZeros32(RegisterID src, RegisterID dest) implementation is missing from mips macro assembler

[Julien Brianceau]

Created attachment 268538 [details]
Add countLeadingZeros32 implementation in mips macro assembler

[Konstantin Tokarev]

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?

[Julien Brianceau]

(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.

[Alex Christensen]

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.

[Julien Brianceau]

Created attachment 268617 [details]
Add countLeadingZeros32 implementation in mips macro assembler (v2)

[Julien Brianceau]

(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

[Konstantin Tokarev]

LGTM

[Konstantin Tokarev]

Could anyone approve this patch?

[Michael Saboff]

Comment on attachment 268617 [details]
Add countLeadingZeros32 implementation in mips macro assembler (v2)

Had to look up the encoding.
r=me

[Julien Brianceau]

Manually committed r195093: http://trac.webkit.org/changeset/195093