Make -msse explicitly require -msimd128

[Akaricchi]

Passing -msse without -msimd128 doesn't make sense anyway, because the SSE intrinsics glue will fail to compile when trying to call the WASM SIMD intrinsics.

More importantly, this unbreaks projects that test for -msse in a configure check, not expecting it to A) target an unstable version of the WASM spec; and B) fail to actually compile the SSE code.

[Akaricchi]

Note that there's one quirk to this: generally if you try to pass flags like -msse2, -mcrc, or some other random target-specific flag that is not relevant to the current target (being WASM in this case), clang will bark a warning and just ignore it:

clang-10: warning: argument unused during compilation: '-mcrc' [-Wunused-command-line-argument]

I consider this to be poor behavior on clang's part, but if you want to be consistent with it, then it probably makes sense to throw a non-fatal warning here instead, too (and not define __SSE__ etc. in that case).

[kripken]

Thanks! lgtm but I may not have followed all the discussion around these flags, @juj @tlively @sbc100

[tlively]

On the one hand it would be nice if -sse implied -msimd128 so users didn't have to pass both, but on the other hand I definitely see the benefit of not allowing configuration scripts to silently target a non-standard feature. Maybe in the future once SIMD support is widespread we can make -sse imply -msimd128.

I would also want to hear any comments from @juj before merging.

[tlively]

Why remove this line? I think this is still relevant.

[kripken]

It's a removal of a duplicate. See 2 lines above.

[juj]

This is not the right way to fix -msse to require -simd128, the BINARYEN_SIMD field controls Binaryen linking stage, and not LLVM's compilation behavior. Proper fix in 8a460f6#diff-b97059c38de7432017d59dee3e87bc72R754

In the "should emcc target SSE with -msse" conversation, it is an argument that one cannot win. I don't mind whichever route we go, willing to accommodate whoever yells the loudest.

[kripken]

I don't think this is about whoever yells the loudest? (maybe that's a joke I don't get?) Let's work together to fix this properly.

Is that link to another PR? (From the commit link it's hard to tell, and so I'm not sure if there's already discussion about it.)

It would be good to resolve this soon as this blocks tagging a new version (since as @Akaricchi pointed out, the current status on master can break existing projects).

[Akaricchi]

That commit seems to be a part of #11290

[kripken]

Thanks @Akaricchi (is there a way I could have seen that from the github UI? maybe I'm missing something obvious...)

Ok, looks like that PR also errors if -msimd128 is missing, so looks like the problem will be solved in the same way by landing either one of them?

I don't have a strong opinion between them myself. But please let's work to agree on one of them, so we can unblock tagging a new release.

[Akaricchi]

(is there a way I could have seen that from the github UI? maybe I'm missing something obvious...)

Nope. I just went looking for a PR that has the SSE3 stuff to find it.

I don't have a strong opinion between them myself. But please let's work to agree on one of them, so we can unblock tagging a new release.

The solution in that PR is fine. But it also contains a new feature (SSE3/SSSE3 support). If you're fine with blocking the release on it, then feel free to close this PR. Alternatively, I can rebase and implement an equivalent just for the -msse and -msse2 flags that are currently in master.

[kripken]

Thanks @Akaricchi , I see.

Ok, looks like the other PR has been merged meanwhile, so I think we are good and can close this.