615489 - (float&float4_types) Add "float" and "float4" datatype to Tamarin

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Virgil Palanciuc]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.44 Safari/534.7
Build Identifier: 

As a first step towards float4 support - add "float" (32-bit floating-point type) to Tamarin.

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: First-cut implementation of FloatClass

First-cut implementation of FloatClass; probably still buggy + nothing implemented in JIT (but it does pass current acceptance tests... on Mac, I didn't test anything else :) ).
Requires that you apply first the bibop patches + the SST patch.

Comment 2

[Virgil Palanciuc]

Attachment: rebased on tamarin-redux 5605:12efad62783c

Comment 3

[Virgil Palanciuc]

Attachment: Float Class - initial patch

initial patch with the float class itself

Comment 4

[Virgil Palanciuc]

Attachment: Float Class - changes to project files

Comment 5

[Virgil Palanciuc]

Attachment: Float Class - parser-related changes

Comment 6

[Virgil Palanciuc]

Attachment: Float Class - verifier-related changes

Comment 7

[Virgil Palanciuc]

Attachment: Float Class - interpreter-related changes

Comment 8

[Virgil Palanciuc]

Attachment: Float Class - codegen changes

Comment 9

[Virgil Palanciuc]

Attachment: Float Class - vector & array support

Comment 10

[Virgil Palanciuc]

In order for this to work, you have to apply first the patches from bugs 616169 and 615871 (these are the nanojit changes), then 626402 (float allocator), 615171 (SST changes). 
(note: some of those patches may be already applied meanwhile - especially 616169 and 615171 which right now have r+ )

The order to apply these patches is:
initial, build, parser, verifier, interpreter, codegen, vector_and_array.

After the "interpreter" patch is applied, the code should compile (before that, the build is broken). At that point, Tamarin works in "interpreter" mode, but breaks if JIT is turned on (even for non-float code). Adding the "codegen" patch makes it work for JITed code too - except the cases where the float is used as an index (array, vectors, objects). Applying the last patch fixes the vectors&arrays. 
Also - check out bugs 657330 (acceptance tests), 657332 (performance tests), 618852 (abcasm/abcdump changes for float support)

Comment 11

[Virgil Palanciuc]

Updates:
* 615171 is now in tamarin-redux, so no need to worry about it.
* If you use mq, you can easily apply the required patches by cloning the patch repository http://asteam.corp.adobe.com/hg/users/virgilp/float_patches (be careful, the last three are float4-related patches, you need to skip those)
* you can skip all the nanojit patches (bugs  616169 & 615871)if you’re only interested in the interpreter – but you will have to make a small modification in LIR.h and add ARGTYPE_F = 5 in enum ArgType{}. You also have to re-generate the files from the “generated” directory – i.e, run core/builtin.py, shell/shell_toplevel.py, and ./generate.py. 
This is not required if you apply bug-615489.floatClass.misc.diff – but that particular patch can only be applied at the very end, because it includes modifications to generated files that are related to the vector class.


* You can find float-specific tests in bugs 657330 (acceptance) and 657332 (performance). To compile those, you need Jeff’s ASC from http://asteam/hg/users/jodyer/asc-redux/, and you need to pass “-abcfuture” to ASC in order to enable float support.

Comment 12

[Steven Johnson]

Comment on attachment 535687 [details] [diff] [review]
Float Class - initial patch

Review of attachment 535687 [details] [diff] [review]:
-----------------------------------------------------------------

Various nits that need fixing; the R- is because I the use of floatAtom() in the Float constructor looks wrong. Am I mistaken?

::: core/FloatClass.cpp
@@ +56,5 @@
> +    {
> +        // Float called as constructor creates new Float instance
> +        // Note: new Float() with no args returns 0 - as does Number
> +        if (argc == 0)
> +            return core()->floatAtom(0);   // can't use zeroIntAtom, this has type 'int' (Number), and we have to return float

Where is AvmCore::floatAtom declared? I don't see it in this patch.

This smells wrong, though -- if floatAtom() accepts a number (e.g "0.0" then the line below where we pass it an Atom is wrong. If it accepts an Atom, then this line is wrong, since "0" is not a valid Atom.

@@ +91,5 @@
> +            minValue = minNormalizedFloat;
> +#ifdef _DEBUG
> +        // It's going to be either 0x1 (1.4013e-45f) if subnormals are supported or 0x00800000 (1.17549e-38f) if not.
> +        union {
> +			float f;

spaces, not tabs.

@@ +102,5 @@
> +    }
> +
> +	Stringp FloatClass::_convert(float n, int precision, int mode)
> +	{
> +		AvmCore* core = this->core();

spaces, not tabs.

@@ +107,5 @@
> +
> +		if (mode == MathUtils::DTOSTR_PRECISION)
> +		{
> +			if (precision < 1 || precision > 21) {
> +				toplevel()->throwRangeError(kInvalidPrecisionError, core->toErrorString(precision), core->toErrorString(1), core->toErrorString(21));

couldn't we collapse these into a single check-and-throw? (ie, use "if mode" to choose upper and lower precision range)

@@ +125,5 @@
> +	{
> +		AvmCore* core = this->core();
> +
> +		if (radix == 10 || MathUtils::isInfinite(fVal) || MathUtils::isNaN(fVal))
> +			return core->doubleToString(fVal);//core->floatToString(fVal);

what's with the rogue floating comment?

::: core/FloatClass.h
@@ +65,5 @@
> +            return construct(argc,argv);
> +        }
> +        
> +        float _minValue();
> +		Stringp _convert(float n, int precision, int mode);

spaces, not tabs

Comment 13

[Virgil Palanciuc]

You're right about floatAtom(0) being wrong - it should've been floatAtom(zeroIntAtom) - my bad. (fwiw the end result is identical, so you can't observe it through tests; but definitely agree that it's conceptually wrong).

The space/tab I *think* is already solved (I didn't re-upload the patch just for that). I'll double-check, anyway.
@DTOSTR_PRECISION: fair point; should I do the change for NumberClass too?
@rogue floating comment: I guess it was meant to say "this is just a micro-optimization, should be floatToString but actually floatToString really calls doubleToString". But yeah, I agree that it's weird, and that micro-optimizations are not a good idea. I'll switch it to "floatToString".

Comment 14

[Steven Johnson]

Comment on attachment 535695 [details] [diff] [review]
Float Class - vector & array support

Review of attachment 535695 [details] [diff] [review]:
-----------------------------------------------------------------

generally fine with mostly nits, but please review the comments in ArrayObject.cpp before landing

::: core/ArrayObject.cpp
@@ +331,5 @@
> +        uint32_t index = uint32_t(f);
> +        if (float(index) == f)
> +              // this is a hot function; explicitly call the force-inlined
> +            // implementation to ensure we don't tail-call to _getUintProperty
> +            return getUintPropertyImpl(index);

this looks like it might be stale: the other calls (eg _getDoubleProperty) now call the vmethod rather than the inline method, so that SemiSealedArrayObject works properly.

@@ +334,5 @@
> +            // implementation to ensure we don't tail-call to _getUintProperty
> +            return getUintPropertyImpl(index);
> +        else
> +            // float is non-integral, negative, or too large to be a valid index, so intern it.
> +        return getStringProperty(core()->internFloat(f));

indentation is wrong

::: core/VectorClass.h
@@ +189,5 @@
> +        }
> +
> +        GC_NO_DATA(FloatVectorClass)
> +
> +            DECLARE_SLOTS_FloatVectorClass;

spaces, not tabs

Comment 15

[Lars T Hansen]

NOTE!

I have updated Virgil's patch queue to update it to TR 6404, make the float4 work compile and run on Mac (still interpreter-only), fix some obvious float4 bugs, and support float in eval (for easier testing).  float4 support is not needed in eval as there's no new syntax.

Comment 16

[Lars T Hansen]

Queue further updated to TR 6417 (floatAllocator patch has landed in TR).

Comment 17

[Lars T Hansen]

Comment on attachment 535690 [details] [diff] [review]
Float Class - parser-related changes

This is generally good.

I will not R+ at this time because I think the patch assumes several things that have yet to be determined:

- Whether there will be a "convert_num" instruction or whether there will simply
  be a "unplus" instruction

- Whether float support can be conditional in the manner described in the 
  comment in AbcParser.h or whether it has to be unconditional, because a 
  new swf can pass data to an old swf.  (Yes, I know that comment is one
  I put in there, so I can blame myself, but even so,)

Nits:

- The new comment in PoolObject.cpp has a typo, "GCFLoat" instead of "GCFloat".

- The definitions of kVERSION_NoExceptionHandlers and kVERSION_FloatSupport
  should just use the shift expressions that are in the comments on the
  definitions, not the hex constants that are currently used.

Also:

- We need a new file in doc/, describing the new file format (note there is
  a file there already for the 46.16 format).  That could be considered part
  of this patch on the same bug but a follow-up patch would be OK.

Comment 18

[Lars T Hansen]

Comment on attachment 535690 [details] [diff] [review]
Float Class - parser-related changes

Stale.

Comment 19

[Lars T Hansen]

Comment on attachment 535692 [details] [diff] [review]
Float Class - interpreter-related changes

Stale.

Comment 20

[Lars T Hansen]

These comments are relative to the patches in the queue as of 28 October 2011.

The "initial" patch:

- Error message in float4 constructor is literal English string, but needs to be
  an error id and there needs to be a string for that id in the error message
  database.

- Ditto error message in float constructor.

- F4OP1 uses __VA_ARGS__, are we sure all our compilers support that now?

- "FIXME" before min, max, reciprocal, _swizzle need to be fixed.

- f4_shuffle is used in this patch but not defined.

- This comment in FloatClass.h is unlikely to be correct:
  "// Note: SpiderMonkey returns 0 for Float() with no args"

- The definitions of some of the methods on float (Number.as) forward to the
  Math methods and I worry about their correctness (ceil, floor, round especially)
  but I don't have a smoking gun.

- core/SIMD.as should really be called core/Float4.as, I think (following the
  convention makes it easier to find things for others).

- Various spacing and typographical errors
  - Float4Class.cpp:
    - missing blank line before construct, dot2
    - nonstandard opening brace in fromComponents (should be on next line)
    - blank line starts body of isGreater
    - spurious backslashes in body of cross()
  - Float4Class.h
    - weird indentation for the declaration of fromComponents
  - FloatClass.cpp:
    - "shoulc" in _doubleToString
  - Number.as:
    - extra blank lines in definition of "float"
    - in general there is variable number of lines between methods in this file
  - SIMD.as:
     - strange and unusual spacing:
       function float4(x:float=0, y:float =0 , z:float =0 , w:float =0 )
     - spurious space before right paren in most method signatures

Comment 21

[Lars T Hansen]

(In reply to Lars T Hansen from comment #20)

> - f4_shuffle is used in this patch but not defined.

Defined in previous patch bug-615871.nJIT_lirasm.diff.

Comment 22

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "build" patch:

project.pbxproj:

The introduction of GCC_PREPROCESSOR_DEFINITIONS is probably inappropriate, this should all be handled with the .xcconfig files if necessary and is.  I don't understand why DEBUGGER shows up here, we handle that elsewhere.  (We have as a rule that we do not muck with the pbxproj more than absolutely necessary, as it quickly becomes unmaintainable.)

MMgc2008.vcproj, avmplus2008.vcproj, eval2008.vcproj, nanojit2008.vcproj, shell2008.vcproj, zlib2008.vcproj:

The introduction of many new preprocessor definitions here are inappropriate, these should be handled with avmshell-features.h.

(Actually lots of changes to the zlib project; why?  Just Visual studio reorging the file?  It's happened before.)

nativegen.py: 

Looks like there's missing code here?

  +            val = "!!! ERROR, float not handled !!!"
  +        elif ct == CTYPE_FLOAT4:
  +            val = "!!! ERROR, float4 not handled !!!"

Comment 23

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "misc" patch:

CdeclThunk.cpp:

Some extra blank lines here.

There is now a standard static_assert() macro that could be used here:

+    /* using MMGC_STATIC_ASSERT for lack of a better one */
+    MMGC_STATIC_ASSERT(kLASTENUMELEMENT <=16); 

Bug: missing code:

+            case BUILTIN_float:
+            case BUILTIN_float4:
+                AvmAssertMsg(false,"Not implemented yet!");
+                break;

And again:

+            case BUILTIN_float:
+            case BUILTIN_float4:
+                AvmAssertMsg(false,"Not implemented yet!");
+                break;

Comment 24

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "parser" patch:

AbcParser.cpp:

Again __VA_ARGS__ macro; do we support this on all compilers?  (Maybe it matters less here, one can compile without AVMPLUS_VERBOSE.)

  +#ifdef AVMPLUS_VERBOSE
  +#define verbose_only(...) __VA_ARGS__
  +#else
  +#define verbose_only(...)
  +#endif

"Cyril" has one "ell":

+            case (47<<16|16):   // Flash player Cyrill

Grumble... it's not great to do the refactoring around constant pool reading as part of this patch set.  And I'm not sure the macro pays for itself; what we want in the parser and verifier code is maximum transparency, and the macro works against that.  The extra-bizarre lingering "else" block for float NaN valus just underscores this.  I would urge undoing some of this work.

Bug: The overflow checking for readFloat4 is incorrect, it needs to check p+15, not p+3.

AbcParser.h:

The type of OP_add also includes XMLList.  This is probably my fault, I suspect I authored the original comment.

+         *  - The type of OP_add is (Number|String|float|float4) while the type

In general this comment block needs to point to the ABC Extensions Spec.

PoolObject.cpp:

This comment is wrong now:

+#ifdef VMCFG_FLOAT
+   // same for float4_t/GCFloat4
+    MMGC_STATIC_ASSERT(sizeof(GCFloat4) == sizeof(float4_t) && sizeof(GCFloat4)%8 == 0 );
+    // GCFLoat is trickier: allocations in heap are 8-byte aligned.
+    // The static assertions here will verify that
+    // GCFloat's size is greater than float, 8-byte aligned, and
+    // the "value" is right at the beginning of a "GCFloat" so that it is 
+    // safe to reinterpret-cast a GCFLoat* to float*.
+    MMGC_STATIC_ASSERT(sizeof(GCFloat)%8==0 && sizeof(GCFloat)>=sizeof(float)
+                       && offsetof(GCFloat,value)==0 );
+#endif 

This looks wrong because it allows floats as default values for Number arguments and vice versa, recall that isNumber tests for "Number or float", see earlier remarks about why this is not a great idea:

             case BUILTIN_number:
                 return AvmCore::isNumber(value);
 
+#ifdef VMCFG_FLOAT
+            case BUILTIN_float:
+                return AvmCore::isNumber(value);
+
+            case BUILTIN_float4:
+                return AvmCore::isFloat4(value);
+#endif

PoolObject.h:

Are these used except in the methods just following them?  If not then they should just be inlined, with comments, in those methods, because the methods provide the useful abstractions.

+        static const int32_t kVERSION_NoExceptionHandlers = 0x002E000F;// (46<<16|15), i.e. major v.46, minor v.15
+#ifdef VMCFG_FLOAT
+        static const int32_t kVERSION_FloatSupport        = 0x002F0010;// (47<<16|16), i.e. Cyrill, major v.47, minor v.16

BTW, +1 on the introduction of hasExceptionSupport.

Comment 25

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "verifier" patch:

FrameState.cpp / FrameState.h:

Since float will land eventually the ifdeffing of the signature on FrameState::FrameState is probably not necessary, it's one of those fixes that could just land as is.

FrameState-inlines.h:

I'm a little concerned about the performance implication of the additonal code in setType.  I don't know how hot this function is, but given that it's a forceinline function it's probably fairly hot.  Followup work here?

Verifier.cpp:

This does not seem appropriate in verifyBlock the way it is there now:

+#ifdef VMCFG_FLOAT
+            if(!pool->hasFloatSupport()){
+                switch(opcode){
+                case OP_convert_f:
+                case OP_convert_f4:
+                case OP_unplus:
+                case OP_pushfloat:
+                case OP_pushfloat4:
+                    verifyFailed(kIllegalOpcodeError, core->toErrorString(info), core->toErrorString(opcode), core->toErrorString((int)(pc-code_pos)));
+                }
+            }
+#endif

Instead it would seem that each case for those instructions should check whether hasFloatSupport() in the big switch.  As it is, every bytecode in pre-47.16 content that is verified in the new VM pays for this test.

The verification error in pushfloat and pushfloat4 in the case where imm30 is zero is in direct contradiction with the spec, which states that this case results in a NaN value and furthermore that since there is no pushnanf one should use index 0.

OP_inclocal/OP_declocal/OP_negate/OP_unplus/OP_increment/OP_decrement:  Suppose the value is some known class type C.  Will we not hit the "!(float||float4||numeric)" case?  If so we hit the ToNumeric runtime conversion which hits ToPrimitive, which invokes public::valueOf on the object, which can return any numeric type that will then be left unchanged by ToNumeric, and ergo the best we can say about the type in
this case is that it will be numeric, yet this code always coerces to number.

On the other hand, if the type is a known primitive type then Number /is/ the correct result because ToNumeric on these types always go to Number.  So there are two cases here that have been collapsed into one.

I think some kind of abstraction here about the conversion algebra (if that's the word for it) would benefit the verifier, the reasoning is bound to be needed more than a few places.

(The binary operators - * / % appear to get this right.)

OP_add: Seems we can and should do better here (this is an ABC spec issue, not an AS3 spec issue).  The fallthrough case generates NUMERIC in the case of FLOAT+INT and FLOAT+UINT.  But in this case the result should be the more specific NUMBER, I think.

OP_modulo, OP_subtract, OP_divide, OP_multiply: style: unconventional formatting of if/else (IMHO, but there's no accounting for taste :-)

Comment 26

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "interpreter" patch:

Interpreter.cpp:

Minor optimization items:

- There has got to be a better way to test for IS_FLOAT and IS_FLOAT4 than a triple conjunction...  And if there isn't then it's a clue that we should be careful not to use IS_FLOAT and IS_FLOAT4 if partial information is available, especially if we're not sure if the compiler will optimize.  (I know, not much guidance in that statement.)

- IS_BOTH_BIBOP has the same issue of course, just worse.  In that case it's possible to optimize the double != test with undefinedAtom in a couple of ways.  Since we know both of them have the right tag we're really only interested in knowing whether the payload of both is nonzero, it seems we should be able to do something with that.  Simply testing "(a ^ kSpecialBibopType) && (b ^ kSpecialBibopType)" may be good enough to allow the compiler to optimize away the branches using
cmov/setcc/etc.

- Another thing that we discussed in the past and that may be worth reviving is to let undefinedAtom be a non-constant and allocating a page to the "undefined" bibop type; something with kSpecialBibopType would never have a zero payload, we could always deref it and get a proper tag.  ISTR there were some complications around that but I forget what they were.

In increment, cf earlier comments about the verifier defaulting to Number rather than Numeric and why that's wrong; the interpreter appears to get it right because the default Number case is not entered
except in the float-not-enabled case.

The assert inserted into increment for int or double could usefully also be inserted into decrement.

Optimization: clearly for the wordcode or other rewriting interpreter we'd wish to special case instructions for when float is and is not enabled in the bytecode.

Probable bugs: I'm not convinced that the adaptations for float4 in MethodEnv::nextname, MethodEnv::nextvalue, MethodEnv::hasnext, MethodEnv::hasnextproto are correct except by accident (if then), this requires further investigation.  What would happen if we were to add an enumerable property to float4 objects?  What if a float4 is the prototype object of another object?

instr.cpp:

The first AvmAssert in toVTable - guarding against undefined - is arguably superflous given the guard on the switch.

In the float4 __modulo case, why is the cast to float necessary?

In op_negate, this seems inappropriate, ie it's a TODO or FIXME masquerading as an assertion:

    AvmAssert(false); // AFAICT, only "float" and "float4" may get here....

IMO it's somewhat accidental that other types are all handled outside op_negate, an effect of optimizations that may change in the future, either permanently or during development (eg think Halfmoon, new interpreter, ...).

Bug: The code for negating float4 appears correct even for -0, but testing for this uncovered that the JIT now breaks the simpler "1/-0": It should print -Infinity but prints NaN instead (Debug Mac32).  So probably a bug introduced in CodegenLIR that we have to look for.  (Or is this an ASC constant folding bug?  I don't think so, seems to be broken even with eval.)

"op_add_legacy" is not a great name because there might be further refinements in the future and "legacy" carries only one bit of information that can only be interpreted in a larger context; op_add_noFloat would probably be a better start.  This comment applies with even greater force later when we get to the DECLARE_OPADD_VERSION cases.

"op_add_a_aa_impl" and "op_add_a_ai_impl" et al: We don't really use "inline", we either use REALLY_INLINE or let the compiler figure it out (compilers are particularly good at that for "static" methods, which these _impl methods can perhaps be?)

Comment 27

[Lars T Hansen]

More comments relative to the queue as of 28 October.

The "vector_and_array" patch:

ByteArray.cpp:

These changes are incorrect according to the spec, there is no readFloat32() specified in the float spec nor is one necessary, with all probability: eventually optimizations will inline readFloat at which point the widening/narrowing conversion pair will also be optimized out.  Until we have the inlining the cost of
widening/narrowing is noise.

ByteArrayGlue.{cpp,h}:

Ditto.

CodegenLIR.cpp:

Is there really any need for the special case where a float is used as an index into a Vector?  I'm sort of doubtful.  The double case is important because so many values in AS3 end up as Number, but in a stricter dialect that has int values that kind of access would simply be a static error.  We could probably simplify by avoiding these accessor methods.

Ditto Arrays, but even more so, I suspect: people who mix Array and float are probably happy as soon as it works and will never care seriously about performance.

Comment 28

[Lars T Hansen]

Comments on the comments:

- Comments were written as patches were reviewed in order, earlier confusions
  may have been recorded in the comments even if cleared up later.  (For example
  I now understand NUMERIC_TYPE.)
- Still working on the codegen patch, which is a big monster
- "supportCode" and "platformSpecificConfig" are not tagged with this bug number
  and comments have been sent by email, comments on patches tagged with other bugs
  have been filed on the other bugs.

Comment 29

[Virgil Palanciuc]

Attachment: Float Class - codegen changes

Comment 30

[Virgil Palanciuc]

Attachment: Float Class - parser-related changes

Comment 31

[Virgil Palanciuc]

Attachment: Float Class - initial patch (class definitions)

Comment 32

[Virgil Palanciuc]

Attachment: Float Class - platform specific defines

Comment 33

[Virgil Palanciuc]

Attachment: Float Class - interpreter-related changes

Comment 34

[Virgil Palanciuc]

Attachment: Float Class - verifier-related changes

Comment 35

[Virgil Palanciuc]

Attachment: Float Class - generic changes (support code)

Comment 36

[Virgil Palanciuc]

Attachment: Float Class - vector & array support (INCOMPLETE)

Comment 37

[Edwin Smith]

Comment on attachment 570694 [details] [diff] [review]
Float Class - codegen changes

_SPF (single) vs _F (double) is confusing.  Lets make CodegenLIR use D suffixes for double, everywhere, before applying this patch.  (nanojit already consistently uses d).

Like the nanojit patch, I'd like to split refactoring from new code.

Overall, I think we can get away with much less #ifdefery, by defining a few variables conditionally as static constants (without float) or variable (with float).  In various places, extra parameters should just be unconditional.  The risk of increasing compile time due to extra parameter passing can be measured, I doubt it will be a problem.

NUMERIC_TYPE seems like a mistake.  abstractly "numeric" is a union type but i worry about it being a nominal type or having a distinct Traits*.  I see there is no BUILTIN_numeric; probably good, but neither should there be numeric_itraits.  I also see that it's a way to sneak the union type of (Number|float|float4) through FrameState and CodegenLIR api's that only have a Traits*, seems dodgy. discuss.

Is NUMERIC_TYPE limited to the cases where we store raw unboxed (Number|float|float4)? i.e inlined if/then/else code in JIT, or a helper which stores to vars[] and tags[], not returning Atom?  If the sst mask of (double|float|float4) can sometimes mean unboxed (and tagged int tags[]) and somtimes mean Atom, then I worry about unsoundness.  I see numerous tests of the value (1<<SST_float4)|(1<<SST_float)|(1<<SST_double) and it makes me worry about cases where two of the 3 bits might be set.  If it can only be those 3 bits, or any of the one other bits, then we need more asserts.

I'd prefer not to ifdef the new varSize parameter to VarTracker.  Elsewhere in codegenlir, we should have a varsize variable.  We can have a single ifdef that defines it as static const or not, for builds without float.  We probably need varsize and varshift variables, and avoid / since int division won't get optimized out.

Somewhere we need a high level comment about how varsize works (8 normally, 16 when F4 static types appear in code, right?)

localGetf() should become localGetd() for double, localGetf() for single-float, and localGetf4() for float4; I don't think we need the precision argument.

where possible, LirHelper functions for specific opcodes should be named for the opcode, with no Ins suffix.  Existing functions with Ins suffixes are legacy.

the numerous places we check for "numeric" need to be abstracted even if it means abstracting other existing type expressions, for consistency. 

jit_sst can always be int16 sized, saving bytes in DEBUG builds doesn't pay for the extra complexity.

> "both are double", "at least one is not Number"m and "both are int"

typo: "m

LirHelper-inlines.h
> REALLY_INLINE LIns* LirHelper::f2f4Ins(LIns* v) {
>   return lirout->ins1(LIR_f2d, v);
> }

Dead code, probably broken (same opcode as f2dIns())

analyze_call() - the case for makeatom should be checked first, before !_isPure, then the duplicated code can be removed.

Comment 38

[Lars T Hansen]

CodegenLIR.cpp:

Really should be using "d" for Number and "f" for float throughout
this file instead of inventing ad-hoc conventions like "SPF" for float
and the precision argument to localGetf/localSetf.  Edwin seems to
think it would be possible to clean up the incorrect "f" uses in
CodegenLIR.  It would cause some ripples, eg VectorDoubleMethodF would
be renamed presumably.

Some spurious spacing changes throughout (in fact the "double space
after ( or before )" seems to be a common issue, here's an example:

-            AvmAssert(!jit_sst[i] || jit_sst[i] == v.sst_mask);
+            AvmAssert(!jit_sst[i] || jit_sst[i] == v.sst_mask  );


Observing that logic in CodegenLIR::localSet duplicates logic in
verifier FrameState (as it should) but that I raised issues about the
cost of the FrameState code, so if we change one we should change the
other.  I don't know if the computational cost matters here for
localSet though.

Also localSet: should we really be making changes like the following?
In Verifier I suggested that the methodinfo should be part of the
framestate unconditionally to avoid too much ifdeffery.  Here it seems
like varSize() is an abstraction that could be introduced
unconditionally, the effects of doing that would ripple through the
code to a fairly large extent.

-        lirout->insStore(o, vars, i * VARSIZE, ACCSET_VARS);
+        lirout->insStore(o, vars, i * IFFLOAT(info->varSize(), VARSIZE) , ACCSET_VARS);

Are assertions like these still pertinent (insLoad, insStore)?

+                FLOAT_ONLY(AvmAssert(!value->isF4() || varSize == 16));

CodegenLIR::writePrologue: I thought the change to 16-bit SST had
landed unconditionally?  Or is this just a size optimization?  How
important is it in practice?

+        jit_sst = new (*alloc1) IFFLOAT(uint16_t,uint8_t)[framesize];
+        memset(jit_sst, 0, framesize FLOAT_ONLY(*sizeof(uint16_t)) );

Spurious comment that should be removed in copyParam:

+                         //  loadIns(LIR_ldp, offset, apArg, ACCSET_OTHER, LOAD_CONST)                          

CodegenLIR::emitNumericOp2: 

- "rslt" might be better phrased as "result" (I'm thinking "right
  shift ... left?")

- emitNumericOp2: If we rely on current atom tags then static
  assertions for those values are required (emitNumericOp2):

+           We *RELY* on the Atom TAG scheme, i.e. "kIntPtrType is 6" and "kDoubleType is 7".

- The following comment is hard to interpret, it should note that it
  gives rise to the "p2i" instruction in both branches of the code
  emitted just after (and also further down in the function, so the
  comment could usefully be before the entire method.)

        // Note that this works on 64bit platforms only if we are careful
        // to restrict the range of intptr values to those that fit within
        // the integer range of the double type.

- Seems like instead of using ldd(subp(p, tag), 0, ...) you could
  use ldd(p, -tag, ...) throughout this function.

CodegenLIR::emitNumericOp1:

- "rslt" again.
- ldd with offset 0 on result of subp again.
- comment about "restricted range" again.

emitAddAtomToAtom: nit: I think you should follow the pattern from the
preceding functions and compute the add function into a local
addFunction variable and then use that, just to make it easy for the
person reading and modifying code.

emitAdd:

- Some indentation bugs
- Redundant assert:

            if (type == FLOAT4_TYPE) {
                AvmAssert( type == FLOAT4_TYPE );

- Strange code, esp given that it's inside code where we've asserted
  that the traits are "Numeric" (though not "NUMERIC").

                AvmAssert(val1.traits==NUMERIC_TYPE);
                if(val1.traits!=NUMERIC_TYPE)
                    localSet(i,coerceToNumeric(i),NUMERIC_TYPE); // this should never happen!
                AvmAssert(val2.traits==NUMERIC_TYPE);
                if(val2.traits!=NUMERIC_TYPE)
                    localSet(j,coerceToNumeric(j), NUMERIC_TYPE);// this should never happen!

write(): This looks incorrect, why not float4 and numeric?  emit()
handles both those cases, and other cases in this same switch also
handle other types.

        case OP_inclocal:
        case OP_declocal:
            AvmAssert(type==NUMBER_TYPE FLOAT_ONLY(|| type == FLOAT_TYPE) );


write(): This is baroque, why not just an extra AvmAssert inside an
#ifdef VMCFG_FLOAT?

+#define EXTRA_CHECKS                                               \
+                (opcode == OP_unplus && type == NUMERIC_TYPE) ||   \
+                (opcode == OP_convert_f && type == FLOAT_TYPE) ||  \
+                (opcode == OP_convert_f4 && type == FLOAT4_TYPE)
+#else
+#define EXTRA_CHECKS 0
+#endif
+
+            AvmAssert( EXTRA_CHECKS ||


coerceToNumeric:

- Is this comment correct?  Should float4 be included (code suggests
  it should)?  If not then why not?

   /** emit code for * -> Float|Number conversion */
    LIns* CodegenLIR::coerceToNumeric(int index)
 
- "rslt" again.
- there's a scary assumption here (correct at the moment, but long run?) that
  every bibop type is a numeric type.

emitIsNaN: If the optimizations in isNaN never kick in at present
  because the value will never be the right type then the code should
  not be in this patch, we should remove the code and attach it to a
  bug to save it.  I've already asserted to PARB that we will perform
  this optimization (mea culpa) because I skimmed the code without
  reading the comment.  It's not a big deal to have the extra
  single->double conversion so we can let it slide until January, but
  consider this work that ought to be done.

  Seems to be connected to the comment in specializedFunctions.

getSpecializedArg: Hidden TODO/FIXME comments?

#ifdef VMCFG_FLOAT
            else if (newBt == BUILTIN_float) {  // not used right now, but maybe soon...
...
           else if (newBt == BUILTIN_number) {  // not used right now, but maybe soon...
                if (arg->isImmF())


emitCall: Comment seems wrong or misleading, since 'int' is always 32
bits; at most it is the "same size as the storage for 'int'":

                disp += sizeof(intptr_t); // same size as an 'int' ! (64 bits on x64)

 
emitCall: Unresolved TODO:

+#ifdef VMCFG_FLOAT
+            case BUILTIN_float: 
+                fid = FUNCTIONID(spfcallimt);// TODO: Test this!!!
+                break;
+            case BUILTIN_float4: 
+                fid = FUNCTIONID(vfcallimt);// TODO: Test this!!!
+                break;
+#endif

emitSetSlot: is this consistent?  Note the FLOAT_ONLY assert in the
second line and the FLOAT_ONLY test in the last line:

        Traits* slotType = tb->getSlotTraits(slot);
        FLOAT_ONLY( AvmAssert( slotType != NUMERIC_TYPE ) );
        if (!slotType || !slotType->isMachineType() || slotType == OBJECT_TYPE )
        {
            // slot type is Atom (for *, Object) or RCObject* (String, Namespace, or other user types)
            const CallInfo *wbAddr = FUNCTIONID(privateWriteBarrierRC);
            if (slotType == NULL FLOAT_ONLY(|| slotType == NUMERIC_TYPE) ||  slotType == OBJECT_TYPE) {

emit: nit: I suspect it would be OK to not emit in-line code for
either float or float4 modulo, the code is big and hairy and ends up
calling out anyway.

Stale comment because the code handles more types now:

    // Faster compares for int, uint, double, boolean
    LIns* CodegenLIR::cmpOptimization(int lhsi, int rhsi, LOpcode icmp, LOpcode ucmp, LOpcode fcmp)

Looks like cmpOptimization does not handle the mixed float4/Number and
float4/float cases for equality but will coerce both to Number?

analye_call: Never assert just "false", either assert like this:
AvmAssert(!"Should not get here because ...")  or use AvmAssertMsg.

                analyze_addp(varPtrArg, vars, varlivein FLOAT_ONLY(, varSize));
            else
                AvmAssert(false);

In deadvars_analyze, should there be cases for LIR_retf4?  If not why
not (ie comment).  Ditto deadvars_kill.

Comment 39

[Lars T Hansen]

(In reply to Lars T Hansen from comment #26)

> ... testing
> for this uncovered that the JIT now breaks the simpler "1/-0": It should
> print -Infinity but prints NaN instead (Debug Mac32).

Unable to reproduce in further, less casual, testing.

Comment 40

[Edwin Smith]

Comment on attachment 570694 [details] [diff] [review]
Float Class - codegen changes

(continued)

deadvars_analyze() (and _kill, probably) needs cases for LIR_callf4 and LIR_retf4.

Comment 41

[Lars T Hansen]

Comment on attachment 570706 [details] [diff] [review]
Float Class - initial patch (class definitions)

R+ with the nits recorded in an earlier comment, to be resolved as decided in the meeting yesterday (see spreadsheet).  FIXMEs are spec issues for now except for the swizzle comment, which needs to be cleaned up.

Comment 42

[Lars T Hansen]

Comments about the "support code" patch of 28 October, previously posted in email:

Somewhere in the basis patches:

Somewhere in an earlier patch there is a definition of "isNumber" as "is Number or is Float".  This seems particularly inappropriate since Number is a type by itself.  Something like "isScalarNumber" or "isNumberOrFloat" is absolutely required to maintain the readability of the code.  (Written after looking at equals() and getting really, really confused.)


AvmCore-inlines.h:

TODO in number_d.

The note about VMCFG_UNALIGNED_FP_ACCESS needs to be merged into the documentation in core/avmfeatures.h.

In allocFloat4 is memcpy ever appropriate?  It seems appropriate only if open-coded by the compiler.  Are we assured of that on all platforms?

Bug: Actually, in allocFloat4, with the latest changes to the allocator that test should never succeed - we always get 16-byte alignment.

"knumeric"?  It's curious that there should be a "numeric" traits at all, is this old code or something very internal?  If very internal it needs better documentation.


AvmCore.cpp:

Nit: dodgy indentation in the signature for handleActionPool (bad before, worse now).

Bug: There is a FIXME in the equals() code, there needs to be code here for mixed-type comparison with float4.  See spec.

Bug: The code for comparing number-or-float with string is incorrect.  The spec specifies that the string should always be converted to Number, never to float (there's even a rationale for it).

In stricteq: "lhs == ltype && rhs == rtype" needs a comment to explain why it's written that way (the comment is in the code), but it seems hat just comparing to "undefined" would accomplish the same at the same speed and the comment would not be needed.

In stricteq: since the types are known to be the same the comparison code for float and float4 can be much better, we only need to load the bibop type and switch on that to find out which branch to take.

In stricteq: isNumber makes another appearance, see above note.

booleanAtom / boolean: why on earth does the former not call the later?  (Not the fault of this patch, but this patch makes it more obvious that some factoring would be appropriate.)

The #ifdeffing around numberAtom / numericAtom is far from ideal, surely there must be some cleaner way of doing this?  There's really no reason numericAtomImpl can't exist also with only numberAtom, and only numericAtom is #ifdef'd.

In general the structure of numericAtomImpl is not good now, the control flow is confusing with some code returning and other code falling through.

Again: number / numberAtom, a lot of duplication here.  Maybe too much to chew over right now.

Bug: The assertion in increment_d, isNumber(*ap), is wrong.  Since isNumber now asserts "int or double or float" (see comments above) it will not catch the case where the parameter is float, yet atomToDouble used in the "else" branch cannot handle float.

The NOTE/WARNING in internFloat needs to be investigated further, and any impact on internFloat4 needs to be assessed as well.

Any reason float4() does not use isFloat4 for its initial test?

"Cyril" is spelled with one ell, not two.

AvmCore.h:

Style: Conventionally this is broken into two lines:

     +        Atom kFltOne, kFltMinusOne;     // needed for increment/decrement

Comment: __m128 struct is probably no longer appropriate here:

     +         * Converts the passed atom to 4 32-bit float numbers (a __m128 struct).

Comment: toNumeric does not in fact have "legacy ABC" behavior, it did not exist

     +        // toNumeric has two behaviours, for "new ABC" and for "legacy ABC"(where it's simply toNumber). 


BuiltinTraits.cpp:

This i find mysterious, what is it for?

+#ifdef VMCFG_FLOAT
+        numeric_itraits = Traits::newTraits(pool, NULL, 0, 0, 0, TRAITSTYPE_NVA);
+        numeric_itraits->set_names(publicNS, core->knumeric);
+        numeric_itraits->final = true;
+        numeric_itraits->builtinType = BUILTIN_any;
+        numeric_itraits->verifyBindings(NULL);
+        numeric_itraits->resolveSignatures(NULL);
+#endif // VMCFG_FLOAT

and later:

+        // numeric is handled above

and later (in BuiltinTraits.h):

+        Traits *numeric_itraits;

MethodInfo-inlines.h:

Nit: Unconventional brace style on MethodInfo::varSize().

NativeFunction.h:

AvmThunkUnbox_float4_impl also tests for alignment of its pointer and uses memcpy if unaligned.  Same questions as earlier: is this required?  and is memcpy a reasonable function to use for data this small?

Toplevel.cpp:

Unresolved FIXME in in_operator.  Note the FIXME comment is itself not correct, we should /not/ do entirely as Vector here because Vector botches the parsing of strings (it accepts all manner of numeric strings, not the limited grammar accepted by Array and required for float4).

  + // FIXME: This is not correct!  We need to do as Vector here.
  + // See VectorBaseObject::getVectorIndex, which should be generalized.

Unresolved TODO in in_operator (checking for "length").

Unresolved FIXME in getproperty: index parsing.

Unresolved FIXME in getproperty: error code and type.

Traits-inlines.h:

Hm, NUMERIC_TYPE, isNumeric vs isNumberType, NUMERIC_TYPE_MASK, NUMBER_TYPE_MASK ...

  - isNumeric is basically int or uint or Number or float or float4
    (and the mask matches this)
  - isNumberType is int or uint or Number (and the mask matches this)

NUMERIC_TYPE is still mysterious though.  What's it for?

The FLOAT_ONLY assert in TraitsBindings::alloc needs a comment (presumably it's to prevent any instantiation of this type?).

Style: is16ByteSlot has non-standard brace style.

in pad16, why not simply this?

  nextSlotOffset = (nextSlotOffset + 15) & ~15;

Style: struct GlueClassTest128_Slots and AlmostStaticAsserts have non-standard brace style (opening brace on next line), ditto methods within.

Traits::visitInitBody: unresolved FIXME.

Traits.h:

Unresolved TODO:

    +    // TODO: AOT support for VMCFG_FLOAT

atom-inlines.h:

It seems to me the change to bibopKind belongs in an earlier patch.

Comment 43

[Lars T Hansen]

Comment on attachment 570710 [details] [diff] [review]
Float Class - generic changes (support code)

A few too many bugs and too many minor issues here for R+, as discussed in review meeting (see spreadsheet).

Comment 44

[Lars T Hansen]

Comments on the "platform specific defines" patch as of 28 October, previously circulated in email:

mac-platform.h:

Any reason the PPC stubs should not just be calls to an abort routine? There could be various abort routines declared as returning float4_t, bool, etc, as need be.

unix_platform.h:

f4_div, f4_eq_i, f4_shuffle: We sort of frown upon the use of "inline", we normally prefer REALLY_INLINE.

Unresolved TODO that should have been FIXME: f4_shuffle is not implemented.

For MIPS, same comment as for PPC: why not use some standard stubs that abort / assert / whatever instead of these macros (which are not the same as on PPC)?

Curious TODO added by this code:

+#elif defined(AVMSYSTEM_ARM) // TODO!!! THIS IS DEFINITELY A BUG, BUT IF I CHANGE TO "IF" THEN LOTS OF PLATFORMS FAIL TO COMPILE!!!

win32-platform.h:

We should discuss these, especially the second one, which I believe we've decided to honor in the past:

+    #pragma warning(disable:4324) // padding was added at the end of a structure because you specified a __declspec(align) value.
+    #pragma warning(disable:4800) // forcing value to bool 'true' or 'false' - bogus performance warning

Comment 45

[Lars T Hansen]

Comment on attachment 570707 [details] [diff] [review]
Float Class - platform specific defines

A few issues to be taken care of - notably around the stubs - and one bug to be filed for abuse of AVMSYSTEM_ARM etc, but basically the patch is sound.  For detailed comments see the review spreadsheet.

Comment 46

[Lars T Hansen]

Comment on attachment 570704 [details] [diff] [review]
Float Class - parser-related changes

Enough bugs to warrant re-review.

Comment 47

[Lars T Hansen]

Comment on attachment 570709 [details] [diff] [review]
Float Class - verifier-related changes

Re-review required due to bugs.

Comment 48

[Lars T Hansen]

Comment on attachment 570715 [details] [diff] [review]
Float Class - vector & array support (INCOMPLETE)

Bugs, incomplete - re-review required.

Comment 49

[Lars T Hansen]

Comment on attachment 570708 [details] [diff] [review]
Float Class - interpreter-related changes

R- because re-review will be needed for op_negate.  The investigation of nextname/nextvalue/hasnext/hasnext2 also feeds into this.

Comment 50

[Edwin Smith]

Comment on attachment 570694 [details] [diff] [review]
Float Class - codegen changes

Overall, I'm in favor of removing as much VMCFG_FLOAT ifdefery as possible. Anything that won't show through to the AS3 semantics should be unconditional.

Comment 51

[Tamarin Bot]

changeset: 6815:699a89753af2
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615489.floatClass.build.diff

http://hg.mozilla.org/tamarin-redux/rev/699a89753af2

Comment 52

[Tamarin Bot]

changeset: 6817:ac405ec7099c
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615489.floatClass.misc.diff

http://hg.mozilla.org/tamarin-redux/rev/ac405ec7099c

Comment 53

[Tamarin Bot]

changeset: 6818:c49fef2bfcd9
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615489.floatClass.parser.diff

http://hg.mozilla.org/tamarin-redux/rev/c49fef2bfcd9

Comment 54

[Tamarin Bot]

changeset: 6819:f79b3b6f77b9
user:      Virgil Palanciuc <[e-mail]>
summary:   imported patch bug-615489.floatClass.verifier.diff

http://hg.mozilla.org/tamarin-redux/rev/f79b3b6f77b9

Comment 55

[Tamarin Bot]

changeset: 6820:0ce2395b86dd
user:      Virgil Palanciuc <[e-mail]>
summary:   * * *

http://hg.mozilla.org/tamarin-redux/rev/0ce2395b86dd

Comment 56

[Tamarin Bot]

changeset: 6821:2556e7a53f72
user:      Virgil Palanciuc <[e-mail]>
summary:   * * *

http://hg.mozilla.org/tamarin-redux/rev/2556e7a53f72

Comment 57

[William Maddox]

Comment on attachment 570694 [details] [diff] [review]
Float Class - codegen changes

Removing stale review request.

Comment 58

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.