Closed Bug 541491 Opened 10 years ago Closed 10 years ago

ARM_ARCH, ARM_VFP, and ARM_THUMB2 should be runtime options everywhere

Categories

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

ARM
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: stejohns, Assigned: stejohns)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)

Attachments

(3 files, 8 obsolete files)

Currently these are compiletime options in TR and runtime options in TM. TR is going to switch to runtime options for these as well, so we should rework these to make that clear (e.g., mirroring the use of config.sse2 on x86)
Attached patch NJ patch (obsolete) — Splinter Review
Attachment #423072 - Flags: review?(nnethercote)
Attachment #423072 - Flags: review?(rreitmai)
Attached patch TM patch (obsolete) — Splinter Review
Attachment #423073 - Flags: review?(nnethercote)
Attached patch TR patch (obsolete) — Splinter Review
Assignee: nobody → stejohns
Attachment #423074 - Flags: review?(rreitmai)
The TR patch is wrong: "arch_default" is typed as bool but should be uint8_t.
If we are touching this much code, maybe we should change the flag to be useFP instead of vfp, then expose it in all builds rather than making it arm specific.
(In reply to comment #5)
> If we are touching this much code, maybe we should change the flag to be useFP
> instead of vfp, then expose it in all builds rather than making it arm
> specific.

Hmm... guess we could, but are we sure the semantics will be the same everywhere? i.e., would this supersede config.sse2 for intel? if so, what happens if/when we add sse3 support?
Or maybe reverse it; disableFP ? If true it means no that floating-point instructions are to be generated.  

sse2 can be looked upon as a fine-tuning of which floating-point instructions are to be generated.
Seems like having that as a generic flag would require writing a bunch more code -- e.g. if you had 'disableFP' as a nj-wide config flag, then there would need to be code for softfloat x86.  Not sure there's much value there, unless softfloat handling moves into NJ itself instead of being handled externally.
Philosophically I disagree with you here, Rick: if we're going to touch a lot of code, we want to make the options as granular as possible. Option flags like this are cheap.
Attachment #423074 - Flags: review?(rreitmai) → review+
Comment on attachment 423072 [details] [diff] [review]
NJ patch


>-        if (!ARM_VFP && (op == LIR_fcall || op == LIR_qcall))
>+#if defined(NANOJIT_ARM)
>+        if (!_config.vfp && (op == LIR_fcall || op == LIR_qcall))
>             op = LIR_callh;
>+#endif

Nit:  I realise it's not part of your change, but the qcall shouldn't be there AIUI -- qcall can't appear on 32-bit platforms, and the operand to callh must be fcall.  (At least, that's what the LIR type-checker requires.)


>-    NanoAssert(ARM_ARCH >= 5);
>+    NanoAssert(config.arch >= 5);

Nit: again, the name pre-existed this patch, but 'arch' is pretty generic, it wasn't clear to me that it was an ARM-only field so I was confused by this until I looked at the declaration of Config.  Maybe 'arm_arch' would be better?


Also, this code in avmplus.h can now be removed, I think?

#ifdef AVMPLUS_ARM
#define ARM_ARCH   AvmCore::config.arch
#define ARM_VFP    AvmCore::config.vfp
#define ARM_THUMB2 AvmCore::config.thumb2
#else
#define ARM_VFP    1
#define ARM_THUMB2 1
#endif


The rest looks good.  r=me with the above considered/addressed.
Attachment #423072 - Flags: review?(nnethercote) → review+
Attachment #423073 - Flags: review?(nnethercote) → review+
BTW, which platforms does SoftFloat apply to -- is it only ARM?  If so, making vfp/useFP/disableFP/whatever visible in all back-ends (as comment 5 suggests) seems like a mistake.
(In reply to comment #10)

> Nit:  I realise it's not part of your change, but the qcall shouldn't be there
> AIUI -- qcall can't appear on 32-bit platforms, and the operand to callh must
> be fcall.  (At least, that's what the LIR type-checker requires.)

Good catch on the qcall -- I don't understand the comment about fcall though. (Sounds like something ripe to be cleaned up in a subsequent patch tho.) I'll convert the test against qcall into an assert.

> >-    NanoAssert(ARM_ARCH >= 5);
>
> Maybe 'arm_arch' would be better?

I was trying to minimize churn against TM, but will happily prefer arm_arch if that's your vote. (Side note: the whole config setup in TM is, frankly, a horrible hack of making stuff look kinda-sorta look like a subset of Tamarin. I'm guessing this is historical fallout from the merge process, but IMHO we should just create our own well-defined config structs...)
 
> Also, this code in avmplus.h can now be removed, I think?

er, yep

> BTW, which platforms does SoftFloat apply to -- is it only ARM?  If so, making
> vfp/useFP/disableFP/whatever visible in all back-ends (as comment 5 suggests)
> seems like a mistake.

I could go either way on this -- I think that historically that was the case (eg, "config.sse2" was only visible in i386) but at some point that was removed in the name of "too many confusing ifdefs in one small space for too little gain". Not sure I agree, but I also don't feel too strongly about it.
(In reply to comment #12)
> 
> I don't understand the comment about fcall though.

See the comment on LIR_callh in LIRopcode.tbl (you'll need a recent NJ copy, though, I think the current TR copy doesn't have the comment).
> BTW, which platforms does SoftFloat apply to -- is it only ARM?  If so, making
> vfp/useFP/disableFP/whatever visible in all back-ends (as comment 5 suggests)
> seems like a mistake.

The philosophy I was taking when making the above comment was to expose features that are non-platform specific to as many architectures as is reasonable.  

The reason I'm suggesting this, is to allow for the code to be more easily tested and debugged.  

I believe this feature is different than others such as 'sse2' in the sense that sse2 implies using an x86 specific instruction set that is not available on other platforms. 

Instead I'd suggest that 'disableFP' is more like the '-Dnocse' flag of tamarin.
 
Now if the work entails a significant effort to support on the other platforms (i.e. more than just removing the ifdefs) or if we don't want to absorb the code size increase, then sure I'll agree, let's not bother.
Comment on attachment 423072 [details] [diff] [review]
NJ patch

Marking + because I don't want to hold up a reasonable patch; but I'd still like to push the removal of ifdef NANOJIT_ARM in CodegenLIR and LIR.
Attachment #423072 - Flags: review?(rreitmai) → review+
Here are a bunch of cpu-specific issues that crop up for "general purpose" softfloat:

* SoftFloatFilter is actually ARM specific in the way it behaves; The ARM ABI requires that doubles passed as arguments to any call be possibly passed in two GP registers.  SoftFloatFilter's insertion of qlo/qhi/qjoin is for this purpose.

* cpu's that have always had FPU support use FPU registers in their ABI.  Includes PPC, X64, and probably MIPS and Sparc too?  Calling a softfloat function with double-typed args would require using that ABI and negate testing value (imo).

Soft float for LIR probably means using strictly no float/double instructions.

We could model cpu-independent soft-float support using int64 values instead of doubles; this would require SoftFloat-like logic for any ABI that split int64 values between parameter registers, but wouldn't have the second problem.  then you'd presumably type-punn the values from int64 to double in the softfloat helper functions, or maybe really write some softfloat code operating on int64 values ... but:
1. There would be little point of this testing on 64bit-cpu's, since all 64bit cpu's of interest have had builtin FPU support since their inception.

2. currently, int64-typed LIR operations are assumed to work only on 64bit cpu's.  which means we'd want to revise the above statement to make softfloat LIR generation to model doubles with 2 int32 values, all over.  If we at least allowed qjoin and 64bit load/store on 32-bit cpu's, this would be much easier.

Anyway, it seems to me that no matter what you'd try, it would be significant new code for the non-ARM cpu's.   So, IMO, softfloat == arm and our flag names could reflect that.
(In reply to comment #16)
> So, IMO, softfloat == arm and our flag names could reflect that.

So let's not generalize for cases that don't currently exist!
Attached patch NJ patch v2 (obsolete) — Splinter Review
Rebased & revised with Nick's comments: LIR_qcall is now banished, and all the arm-specific settings are prefixed with "arm_", and redundant #defines removed.
Attachment #423072 - Attachment is obsolete: true
Attachment #423437 - Flags: review?(nnethercote)
Attached patch TM patch, v2 (obsolete) — Splinter Review
Note: this has been built & tested on x86 and x64, but not on ARM, as I don't have a TM build env for that. Someone should probably build & test on ARM before pushing.
Attachment #423073 - Attachment is obsolete: true
Attachment #423438 - Flags: review?(nnethercote)
Attached patch TR patch, v2 (obsolete) — Splinter Review
Attachment #423074 - Attachment is obsolete: true
Attachment #423439 - Flags: review?(rreitmai)
Attachment #423437 - Flags: review?(rreitmai)
Comment on attachment 423437 [details] [diff] [review]
NJ patch v2

>@@ -223,40 +214,40 @@
> #if defined (AVMPLUS_ARM)
>         // Whether or not to generate VFP instructions.
> # if defined (NJ_FORCE_SOFTFLOAT)
>-        static const bool vfp = false;
>+        static const bool arm_vfp = false;
> # else
>-        bool vfp;
>+        bool arm_vfp;
> # endif
> 
>         // The ARM architecture version.
> # if defined (NJ_FORCE_ARM_ARCH_VERSION)
>-        static const unsigned int arch = NJ_FORCE_ARM_ARCH_VERSION;
>+        static const unsigned int arm_arch = NJ_FORCE_ARM_ARCH_VERSION;
> # else
>-        unsigned int arch;
>+        unsigned int arm_arch;
> # endif
> 
>         // Support for Thumb, even if it isn't used by nanojit. This is used to
>         // determine whether or not to generate interworking branches.
> # if defined (NJ_FORCE_NO_ARM_THUMB)
>-        static const bool thumb = false;
>+        static const bool arm_thumb = false;
> # else
>-        bool thumb;
>+        bool arm_thumb;
> # endif
> 
>         // Support for Thumb2, even if it isn't used by nanojit. This is used to
>         // determine whether or not to use some of the ARMv6T2 instructions.
> # if defined (NJ_FORCE_NO_ARM_THUMB2)
>-        static const bool thumb2 = false;
>+        static const bool arm_thumb2 = false;
> # else
>-        bool thumb2;
>+        bool arm_thumb2;
> # endif
> 
> #endif
> 
> #if defined (NJ_FORCE_SOFTFLOAT)
>-        static const bool soft_float = true;
>+        static const bool arm_soft_float = true;
> #else
>-        bool soft_float;
>+        bool arm_soft_float;
> #endif
>     };

This last field doesn't fall within the '#if defined (AVMPLUS_ARM)' above.
If comment 17 is correct and SoftFloat really is an ARM-only thing (which
the "arm_" prefix definitely implies!), then it should be moved within the
scope of the #if.

r=me with that addressed.
Attachment #423437 - Flags: review?(nnethercote) → review+
Comment on attachment 423438 [details] [diff] [review]
TM patch, v2

Perhaps the SoftFloatFilter in jstracer.cpp and CodegenLIR.cpp should be surrounded with #if defined NANOJIT_ARM as well.  (Fodder for a follow-up bug.)
Attachment #423438 - Flags: review?(nnethercote) → review+
I tried to keep my changes to TM code as minimal as I could. That said, I'm inclined to agree with Edwin that SoftFloat seems likely to be ARM-only for the forseeable future; as handy as it would be to be able to enable it on (say) x86 for debugging purposes, I don't think it would be close enough to be meaningful. So for now I'm including to move "arm_soft_float" into the ifdef ARM case.

(Note 1: "arm_soft_float" is TM only; TR just assumes that ARM && !VFP means softfloat.)

(Note 2: will MIPS ever likely need softfloat support? I really don't know the prevalence of FPU support there.)
(In reply to comment #23)
> (Note 2: will MIPS ever likely need softfloat support? I really don't know the
> prevalence of FPU support there.)

From offline discussion with Rick, he suggests that MIPS might need softfloat support. So I will just change "arm_soft_float" back to "soft_float" for now, which I think satisfies your request.
Comment on attachment 423437 [details] [diff] [review]
NJ patch v2

Ed argues in comment #16 that its effort to generalize and I concur, ARM only is fine; +'ing
Attachment #423437 - Flags: review?(rreitmai) → review+
Attachment #423439 - Flags: review?(rreitmai) → review+
http://hg.mozilla.org/projects/nanojit-central
Whiteboard: fixed-in-nanojit
Attached patch TM Patch, v3 (obsolete) — Splinter Review
Same as previous, but with "soft_float" instead of "arm_soft_float"
Attachment #423438 - Attachment is obsolete: true
Attachment #423565 - Flags: review?(nnethercote)
http://hg.mozilla.org/tamarin-redux/rev/7a4c8c33bf4b
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin
Adds "-Darm_arch N" and "-Darm_vdfp" to avmshell to allow testing with difference configurations.
Attachment #423601 - Flags: review?(rreitmai)
Comment on attachment 423565 [details] [diff] [review]
TM Patch, v3

The TM patch is fine.  But I'm now confused about whether 'soft_float' is #ifdef NANOJIT_ARM or not.
Attachment #423565 - Flags: review?(nnethercote) → review+
(In reply to comment #30)
> (From update of attachment 423565 [details] [diff] [review])
> The TM patch is fine.  But I'm now confused about whether 'soft_float' is
> #ifdef NANOJIT_ARM or not.

soft_float is *not* ifdef ARM; there is a possibility that MIPS may also need to support it.

I don't think have push right for TM, can you push it for me?
(In reply to comment #31)
> 
> I don't think have push right for TM, can you push it for me?

I'll do it with the next NJ-to-TM merge, as per usual procedure.
Comment on attachment 423601 [details] [diff] [review]
Add commandline flags to avmshell

i assume the default_ values are already in the code.
Attachment #423601 - Flags: review?(rreitmai) → review+
Do we really need a separate flag for Thumb2? All ARMv7+ devices support it (so arm_arch >= 7 ought to enable arm_thumb2 by default), and AFAIK only one earlier variant supported it.
(In reply to comment #35)
> Do we really need a separate flag for Thumb2? All ARMv7+ devices support it (so
> arm_arch >= 7 ought to enable arm_thumb2 by default), and AFAIK only one
> earlier variant supported it.

Yep, you're right. It appeals to my nit-picking side, but I can't see any problem with excluding ARMv6T2; the only implementation I'm aware of is ARM1156, which we aren't likely to encounter. (Even if we do, the code will still work without the new instructions.)

Indeed, the run-time detection (on Linux at least) can't detect ARMv6T2 anyway, so on that platform ARM_THUMB2 is equivalent to "ARM_ARCH >= 7".
Out of curiosity, what existing device(s) are using ARMv6T2/ARM1156? (I'm presuming that it's unlikely many new devices will use it at this point in time, at least, any new devices that we're interested in targeting...)
(In reply to comment #36)
> Yep, you're right. It appeals to my nit-picking side, but I can't see any
> problem with excluding ARMv6T2; the only implementation I'm aware of is
> ARM1156, which we aren't likely to encounter. 

FYI, my main interest here is to make it easy for embedding code to get the best options -- right now it's easy to set arm_arch=7 but neglect to set arm_thumb2=true.

If we think there's any chance we may want to enable/disable thumb2 separately from anything else in the forseeable future, I vote we keep the thumb2 flag but quietly set it to true if arm_arch>=7.

If not, perhaps we just lose the flag and use arm_arch>=7 for the places (well, currently exactly one place) we use arm_thumb2.
http:///hg.mozilla.org/tracemonkey/rev/cf0ba05a0ef9
http:///hg.mozilla.org/tracemonkey/rev/504415c9f843
Whiteboard: fixed-in-nanojit, fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey
> Out of curiosity, what existing device(s) are using ARMv6T2/ARM1156?

It's designed as a real-time core, so it turns up embedded in things like communications devices, hard disk drivers and the like. I don't know any specific examples. We are very unlikely to see it.
Attachment #423437 - Attachment is obsolete: true
Attachment #423439 - Attachment is obsolete: true
Attachment #423565 - Attachment is obsolete: true
Attachment #423601 - Attachment is obsolete: true
Removes both the arm_thumb and arm_thumb2 flags. 

arm_thumb is unused; nanojit doesn't generate thumb-specific code and isn't likely to anytime soon. 

arm_thumb2 is used in exactly one place, but (as discussed below) simply checking for ARMv7 or later seems simpler and safer.
Attachment #424086 - Flags: review?(nnethercote)
Attachment #424086 - Flags: review?(rreitmai)
Attachment #424087 - Flags: review?(nnethercote)
Attachment #424088 - Flags: review?(rreitmai)
Attachment #424088 - Flags: review?(rreitmai) → review+
Attachment #424086 - Flags: review?(rreitmai) → review+
Comment on attachment 424086 [details] [diff] [review]
Remove thumb and thumb2 flags

>     //  - We cannot use MOV(W|T) on cores older than the introduction of
>     //    Thumb-2 or if the target register is the PC.
>-    if (config.arm_thumb2 && (d != PC)) {
>+    // (Note that use Thumb2 if arm_arch is ARMv7 or later; the only earlier
>+    // ARM core that provided Thumb2 is ARMv6T2/ARM1156, which is a real-time
>+    // core that nanojit is unlikely to ever target.)
>+    if (config.arm_arch >= 7 && (d != PC)) {

Nit: the wording of the comment is awkward/unclear ("Note that use Thumb2"?)

r=me with that fixed.  No need to re-review.

Nb: I don't much like posting multiple patches on the same bug;  it makes log messages and merge tracking difficult.  I particularly dislike marking committed patches as obsolete.  Can we open new bugs for new patches in the future please?  That's standard Mozilla practice.
Attachment #424086 - Flags: review?(nnethercote) → review+
Attachment #424087 - Flags: review?(nnethercote) → review+
(In reply to comment #44)
> Nit: the wording of the comment is awkward/unclear ("Note that use Thumb2"?)

Whoa. Yeah. I'll translate into something approximating English prior to pushing.

> Nb: I don't much like posting multiple patches on the same bug;  it makes log
> messages and merge tracking difficult.  I particularly dislike marking
> committed patches as obsolete.  Can we open new bugs for new patches in the
> future please?  That's standard Mozilla practice.

I thought this was related-enough to tag along on this bug, but yep, new bugs are cheap. I'll strive for more granularity in the future.
(In reply to comment #45)
> > Nit: the wording of the comment is awkward/unclear ("Note that use Thumb2"?)
> 
> Whoa. Yeah. I'll translate into something approximating English prior to
> pushing.

Did you forget?  The committed version looks the same.
(In reply to comment #47)
> Did you forget?  The committed version looks the same.

Sigh -- I didn't forget to fix it, I just forgot to reapply the proper patch :-/

I'll fix it for real, sorry about that.
LirBufWriter::insCall() has this code:

  #if defined(NANOJIT_ARM)
        if (!_config.arm_vfp && op == LIR_fcall)
            op = LIR_callh;
  #endif  

That's really SoftFloat-specific, not ARM-specific, right?  So this would be better:

  #if NJ_SOFTFLOAT_SUPPORTED
        if (_config.soft_float && op == LIR_fcall)
            op = LIR_callh;
  #endif

(I don't like having both arm_vfp and soft_float, it's a redundancy.)

I also don't understand the code that follows shortly after:

  #ifndef NANOJIT_64BIT
        ins->initLInsC(op==LIR_callh ? LIR_icall : op, args2, ci);
  #else
        ins->initLInsC(op, args2, ci);
  #endif

Looks like we carefully change fcall into callh, then change callh into icall?
(In reply to comment #50)
>   #if NJ_SOFTFLOAT_SUPPORTED
>         if (_config.soft_float && op == LIR_fcall)
>             op = LIR_callh;
>   #endif

Sure, except that "_config.soft_float" only exists in TM right not, not in TR. (See also https://bugzilla.mozilla.org/show_bug.cgi?id=542133)
 
> (I don't like having both arm_vfp and soft_float, it's a redundancy.)

TR doesn't have both :-)
 
> I also don't understand the code that follows shortly after:

Got me, that code predates me. Rick or Edwin might know.
Status: NEW → ASSIGNED
Flags: blocking-flashplayer10+
Priority: -- → P2
Target Milestone: --- → flash10.1
http://hg.mozilla.org/mozilla-central/rev/235c7eaf8541
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.