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

Fixed compilation on compilers that use EDG C++ frontend #587

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kosumosu
Copy link

The Edison Design Group frontend used in MCST lcc compiler as well as Intel C++ compiler and some others. We faced a problem when lcc was complaining that "no suitable conversion function from "__m128" to "float"". It could not digest uniform construction of _m128 from another _m128 due to a bug in the EDG frontend.
So here I replaced uniform initialization of __m128 and __m128i with assignment.

… since EDG frontend thinks it's just plain float.
@kcat
Copy link
Owner

kcat commented Jul 23, 2021

I'd prefer not to do assignment initialization of values, since that allows implicit conversions and silent truncation. Is the EDG frontend expecting that bug to get fixed? Initializing a value with another of the same type is a pretty common thing, and is the preferred way to initialize things by the C++ Core Guidelines, so I doubt OpenAL Soft would be the only thing affected by this.

@leha-bot
Copy link

@daveedvdv sorry for mentioning you, but could you help us with EDG frontend bug related to initialization?

@r-a-sattarov
Copy link

I'd prefer not to do assignment initialization of values, since that allows implicit conversions and silent truncation. Is the EDG frontend expecting that bug to get fixed? Initializing a value with another of the same type is a pretty common thing, and is the preferred way to initialize things by the C++ Core Guidelines, so I doubt OpenAL Soft would be the only thing affected by this.

I do not know about this bug, but besides this there is another bug in EDG (related to Intel Intrinsics).

@helce
Copy link

helce commented Jul 27, 2021

We opened EDGcpfe/21800 for this issue in 2019, no results yet from them...

@a1batross
Copy link

@kcat I doubt this will be fixed in EDG any time soon.

What's the actual difference these syntax sugar for initialization? I don't think the implicit initialization is possible for SIMD vector types.

@daveedvdv
Copy link

I bumped the priority of EDGcpfe/21800. I'll try to get to it soonish...

@r-a-sattarov
Copy link

I bumped the priority of EDGcpfe/21800. I'll try to get to it soonish...

Please also look at EDGcpfe/24405. This is from the same situations with Intel Intrinsics. Thank you!

@daveedvdv
Copy link

Will do. (That one was not assign to me, but it looks related. So I took it over and bumped it as well.)

@kcat
Copy link
Owner

kcat commented Aug 13, 2021

What's the actual difference these syntax sugar for initialization? I don't think the implicit initialization is possible for SIMD vector types.

Type var{...}; is uniform initialization, and is the recommended way to initialize variables since C++11. It does not allow narrowing (e.g. char c{some_int}; is an error; you need to explicitly cast the int to char with static_cast), so it avoids surprise data loss, and is generally slightly more strict with conversions.

Type var(...); is the old style initialization, and is more lax on its conversion rules (e.g. char c(some_int); is allowed without issue, even though it's losing 3/4ths of its data). Also there's some cases where it's syntactically ambiguous if you're defining a variable or declaring a function, and the compiler can do the wrong thing.

Type var = ...; is the same as above (old style, lax conversions), but also could be mistaken for an assignment with the = operator to a casual observer. There may also be pitfalls where you get surprise temporaries generated for initialization, I think.

As far as OpenAL Soft goes, the only time Type var = ...; is used is for references and/or auto. This is in part due to early C++11 deficiencies (fixed in later spec updates, and not an issue since C++14), and something I've not broken the habit of doing. But also since auto will at most decay the initializer type, implicit conversions and data loss aren't as much of a concern (const or rvalue references with non-auto types, though, do raise that concern).

@daveedvdv
Copy link

I checked in fixes for EDGcpfe/21800 and EDGcpfe/24405. They still need to work their way through our QA pipeline, of course.

@stgatilov
Copy link

To me it looks like a bug of OpenAL.
Intrinsics are not a part of C++, not even a part of C. One must not expect all sort of C++ stuff to work with them.

I'd definitely vote for merging the fix into OpenAL.

P.S. I think pushing such"C++ish" style into a low-level library like OpenAL without real need is bad direction =(

@kcat
Copy link
Owner

kcat commented Aug 16, 2021

Intrinsics are not a part of C++, not even a part of C. One must not expect all sort of C++ stuff to work with them.

It's basic C++ syntax for variable initialization. It's using the value in the {...} brackets to initialize the variable being declared, functionally equivalent to = except with stricter compile-time type checking. It's perfectly reasonable I think for SSE intrinsics to work with standard initialization syntax.

P.S. I think pushing such"C++ish" style into a low-level library like OpenAL without real need is bad direction =(

The need comes from stricter type handling and better compile-time error checking. C is extremely lax with type conversions, which gets compounded by strict aliasing for pointers. C also has a lack of features regarding polymorphism and generic type encapsulation, requiring messy code (macros, unsafe casts, type erasure) to get such functionality. Before the switch to C++ I had to hope the compiler wouldn't mis-optimize various pieces of code because of type-punning (particularly around generic interfaces for the backends and effects, atomics, or dynamic storage), while at the same time I didn't want it to miss optimizations thinking two incompatible pointer types might alias. The "C++ish style" helps the compiler understand the intent of the code, more properly compile it, and be more prone to warn or error when it's being used incorrectly.

I am careful to ensure the code's not doing work it shouldn't be in the core real-time/mixing functions, that performance-minded functions can optimize just as well as if written in C (if not better), and there's a balance between CPU/memory use and maintainable code for the rest. And I'm open to improvements in these areas where anyone spots an issue.

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.

8 participants