Closed Bug 763378 Opened 9 years ago Closed 9 years ago

arm detection should have correct define when -mfpu=neon on gcc or on MSVC

Categories

(Core :: XPCOM, defect)

ARM
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: m_kato, Unassigned)

Details

Attachments

(1 file)

- When using -mfpu=neon, we should define MOZILLA_PRESUME_NEON=1
- On MSVC, _M_ARM is cpu architecture.  We should not be always 3 for MOZILLA_ARM_ARCH on CPU.
Attached patch fixSplinter Review
Attachment #631805 - Flags: review?(tterribe)
Comment on attachment 631805 [details] [diff] [review]
fix

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

(In reply to Makoto Kato from comment #0)
> - When using -mfpu=neon, we should define MOZILLA_PRESUME_NEON=1

Should we also #define MOZILLA_PRESUME_ARMV6 and MOZILLA_PRESUME_EDSP in this case?

::: xpcom/glue/arm.h
@@ +72,5 @@
>  #      define MOZILLA_MAY_SUPPORT_NEON 1
>  #    endif
>  #  endif
>  
> +  // when using -mfpu=neon, gcc generates neon instruction.

Capital letter on "when", please. Also "instruction" should be "instructions".
Attachment #631805 - Flags: review?(tterribe) → review+
(In reply to Timothy B. Terriberry (:derf) from comment #2)
> Comment on attachment 631805 [details] [diff] [review]
> fix
> 
> Review of attachment 631805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Makoto Kato from comment #0)
> > - When using -mfpu=neon, we should define MOZILLA_PRESUME_NEON=1
> 
> Should we also #define MOZILLA_PRESUME_ARMV6 and MOZILLA_PRESUME_EDSP in
> this case?

new ARMv8 supports NEON, but architecture isn't compatible with ARMv6.  So I don't think so.
(In reply to Makoto Kato from comment #3)
> new ARMv8 supports NEON, but architecture isn't compatible with ARMv6.  So I
> don't think so.

Hmm? I thought AArch32 was fully backwards-compatible with ARMv7, including the ARMv6 "media" instructions (which is all MOZILLA_PRESUME_ARMV6 implies). But I suppose we can defer this until there are actual ARMv8 toolchains and chips.
(In reply to Timothy B. Terriberry (:derf) from comment #4)
> (In reply to Makoto Kato from comment #3)
> > new ARMv8 supports NEON, but architecture isn't compatible with ARMv6.  So I
> > don't think so.
> 
> Hmm? I thought AArch32 was fully backwards-compatible with ARMv7, including
> the ARMv6 "media" instructions (which is all MOZILLA_PRESUME_ARMV6 implies).
> But I suppose we can defer this until there are actual ARMv8 toolchains and
> chips.

If 64bit arm, no AArch32, __ARM_NEON__ may be defined.  But, of course, ARMv8 gcc/binutils doesn't release yet...
https://hg.mozilla.org/mozilla-central/rev/63a91a66c14c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.