Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it usable on non-SSE platforms #60

Merged
merged 1 commit into from Jun 10, 2017

Conversation

DanielGibson
Copy link
Contributor

For Yamagi Quake2 we had a bugreport that it doesn't build on ARM due to HMM, so I looked into the issue and (hopefully!) fixed it

  • at one place HANDMADE_NO_SSE instead of HANDMADE_MATH_NO_SSE was used
  • introduce HANDMADE_MATH__USE_SSE for the SSE #ifdefs throughout code
    • use #ifdef HANDMADE_MATH_NO_SSE at only one place
  • only use SSE (#define HANDMADE_MATH__USE_SSE) if the targetplatform
    actually supports it
    => users don't have to #define HANDMADE_MATH_NO_SSE on ARM etc
  • at one place HMM_SqrtF instead of HMM_SquareRootF was used

* at one place HANDMADE_NO_SSE instead of HANDMADE_MATH_NO_SSE was used
* introduce HANDMADE_MATH__USE_SSE for the SSE #ifdefs throughout code
  - use #ifdef HANDMADE_MATH_NO_SSE at only one place
* only use SSE (#define HANDMADE_MATH__USE_SSE) if the targetplatform
  actually supports it
  => users don't have to #define HANDMADE_MATH_NO_SSE on ARM etc
* at one place HMM_SqrtF instead of HMM_SquareRootF was used
DanielGibson added a commit to yquake2/yquake2 that referenced this pull request Jun 9, 2017
@bvisness
Copy link
Member

bvisness commented Jun 9, 2017

Looks good to me, but we'll see what @StrangeZak thinks. I'm just confused why the tests didn't catch the typo with HMM_SqrtF.

@DanielGibson
Copy link
Contributor Author

my assumption would be that you have no tests without SSE?

@bvisness
Copy link
Member

bvisness commented Jun 9, 2017

Oh yeah, that's right...my mental todo list is coming back to me now :P

@bvisness
Copy link
Member

bvisness commented Jun 9, 2017

I'll get those test cases added to make sure this stuff doesn't come up again.

@DanielGibson
Copy link
Contributor Author

Great!

Ideal would be tests (or at least builds) for non-x86 to make sure it builds at all without SSE (ideally without having to manually specify HANDMADE_MATH_NO_SSE; my patch should allow that).

But I don't know if travis or another free service offers other platforms (ARM?)

@bvisness
Copy link
Member

bvisness commented Jun 9, 2017

I looked at my own code and was surprised to see that it was already testing with and without SSE...only to find that it's on the SSE branch which has been idle for a long time. I'll pull over the relevant testing fixes for now and @StrangeZak's other SSE stuff can come later whenever he decides to finish it.

Unfortunately I am already testing with both compilers available to me on Travis. It seems Travis's support for C and C++ is pretty limited compared to their other platforms. Might there be some more gcc and g++ options that could test different architectures? I would guess it wouldn't handle different includes anyway...

@DanielGibson
Copy link
Contributor Author

DanielGibson commented Jun 9, 2017

just gcc/g++ options won't suffice, but maybe there are crosscompilers available, e.g. Ubuntu has the gcc-5-arm-linux-gnueabi package.

Note that I consider the change from HANDMADE_MATH_NO_SSE throughout the code to only checking it once and then use HANDMADE_MATH__USE_SSE instead important - for once you don't have to specify anything on non-x86 and it still works, but this also allows adding other kinds of SIMD code (e.g. ARM NEON) in the code (with sth like HANDMADE_MATH__USE_NEON) later without going insane - I mean, how would the #ifdefs look like then?

#ifdef HANDMADE_MATH_NO_SSE
#if defined(__arm__) && !defined(HANDMADE_MATH_NO_NEON)
// neon code
#else
// C code
#endif // __arm__ or whatever
#else // not HANDMADE_MATH_NO_SSE
// SSE code
#endif

?? horrible.
with my change it'd be like

#ifdef HANDMADE_MATH__USE_SSE
// SSE code
#elif defined(HANDMADE_MATH__USE_NEON)
// NEON code
#else
// plain C code
#endif

@bvisness
Copy link
Member

bvisness commented Jun 9, 2017

Yeah, I've questioned the use of SSE-by-default in this repo before. I think the USE_SSE approach makes more sense.

I just remembered that Handmade Network projects get Gitlab projects, and Gitlab has very robust CI, so we might be able to do much more robust testing over there if we ever migrated the repo. Github is convenient and good for visibility though.

@DanielGibson
Copy link
Contributor Author

TBH, if this project were on some gitlab server somewhere and I'd have to create an account there to do a PR, I'd be less likely to do that.
Here on github it's easy (as I already have an account), but otherwise not sure I'd bother.

(Of course you could just mirror it there and keep it in sync)

@StrangeZak
Copy link
Member

Super good change grabbing it now!

@StrangeZak StrangeZak closed this Jun 10, 2017
@StrangeZak StrangeZak reopened this Jun 10, 2017
@StrangeZak StrangeZak merged commit ff4513f into HandmadeMath:master Jun 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants