Bug 31060 - Use ARMv7 specific encoding for immediate constants on ARMv7 target
Summary: Use ARMv7 specific encoding for immediate constants on ARMv7 target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-03 07:39 PST by Gabor Loki
Modified: 2009-11-05 00:28 PST (History)
2 users (show)

See Also:


Attachments
Use ARMv7 specific encoding for immediate constants on ARMv7 target (4.33 KB, patch)
2009-11-03 07:40 PST, Gabor Loki
barraclough: review-
Details | Formatted Diff | Diff
Use ARMv7 specific encoding for immediate constants on ARMv7 target (take 2) (5.82 KB, patch)
2009-11-04 04:03 PST, Gabor Loki
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Loki 2009-11-03 07:39:21 PST
ARMv7 provides a simple way to store 16bit wide immediate constants to a register. This is useful when JavaScriptCore is tuned to an ARMv7 processor.
Comment 1 Gabor Loki 2009-11-03 07:40:22 PST
Created attachment 42381 [details]
Use ARMv7 specific encoding for immediate constants on ARMv7 target
Comment 2 Gavin Barraclough 2009-11-04 01:45:33 PST
Comment on attachment 42381 [details]
Use ARMv7 specific encoding for immediate constants on ARMv7 target

Having 0 as the "cannot encode" value for getImm16Op2 is probably not a good idea, since 0 is a common immediate, and could be encoded in one instruction.

I guess this is not actively a problem in this patch, since you don't call encodeComplexImm on zero – but to guard against this mistake creeping in you should probably ASSERT(imm != 0) in this function, or use a value other than 0 to mean 'cannot encode'.

We also prefer to avoid magic values in the code, so I'd suggest that you should define up a name for this (e.g. something like static const ARMWord INVALID_IMM16 0;), then return 0; should be return INVALID_IMM16;, if (tmp) should be if (tmp != INVALID_IMM16).

It would also probably be a good idea to add guards to movw_r & movt_r that op2 is valid.

r-, because I think some extra ASSERTs here are worthwhile, but the patch is otherwise all good.
Comment 3 Gabor Loki 2009-11-04 04:03:15 PST
Created attachment 42474 [details]
Use ARMv7 specific encoding for immediate constants on ARMv7 target (take 2)
Comment 4 Zoltan Horvath 2009-11-05 00:28:54 PST
Landed in 50553.
https://trac.webkit.org/changeset/50553