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

[MLIR] mlir/include/mlir/IR/BuiltinAttributes.h(354,14): warning: 'complex' is deprecated: warning STL4037: #65255

Open
vhyijk opened this issue Sep 4, 2023 · 11 comments
Labels
code-quality mlir:core MLIR Core Infrastructure

Comments

@vhyijk
Copy link

vhyijk commented Sep 4, 2023

Host & Target: x86_64-pc-windows-msvc
LLVM Version: 16.0.6

D:/llvm-project-16.0.6.src/mlir/include/mlir/IR/BuiltinAttributes.h(354,14): warning: 'complex' is deprecated: warning STL4037: The effect of instantiating the template std::complex for any type other than float, double, or long double is unspecified. You can define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to suppress this warning. [-Wdeprecated-declarations]
      return {APFloat(*smt, value.real()), APFloat(*smt, value.imag())};
             ^
D:\Program Files\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.37.32822\include\complex(1302,5): note: 'complex' has been explicitly marked deprecated here
    _DEPRECATE_NONFLOATING_COMPLEX
    ^
D:\Program Files\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.37.32822\include\yvals_core.h(1408,7): note: expanded from macro '_DEPRECATE_NONFLOATING_COMPLEX'
    [[deprecated("warning STL4037: "                                                   \
      ^
1 warning generated.
@EugeneZelenko EugeneZelenko added mlir:core MLIR Core Infrastructure code-quality and removed new issue labels Sep 4, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2023

@llvm/issue-subscribers-mlir-core

@joker-eph
Copy link
Collaborator

We've been using std::complex<APInt> / std::complex<APFloat> for a while, it'll take time to clean this up

@DavidTruby
Copy link
Member

I wonder if in the mean time it might be worth to silence the warning with -D_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING? Just so Windows builds aren't spammed with these warnings

@joker-eph
Copy link
Collaborator

Not opposed, but I'm just concerned this will hide the issue and we won't fix it.
Do we run the risk that a MSVC update actually breaks up?

@DavidTruby
Copy link
Member

It's a C++ standard definition that std::complex can only be used with certain types, not an MSVC thing specifically. Theoretically any implementation could just break this at any time; whether they will or not in practice is another matter!
MSVC tend to be more aggressive in breaking things like this than the others though, which is probably why they added the warning.

@aganea
Copy link
Member

aganea commented Oct 2, 2023

Not opposed, but I'm just concerned this will hide the issue and we won't fix it. Do we run the risk that a MSVC update actually breaks up?

The standard prohibits custom floating types:

28.4.1/2/ The effect of instantiating the template complex for any type that is not a cv-unqualified floating-point type (6.8.2) is unspecified.

APFloat is not a fundamental floating type, as expected by 6.8.2.

SO link from 2017: https://stackoverflow.com/questions/11108743/why-does-c-mandate-that-complex-only-be-instantiated-for-float-double-or-lon

Perhaps someone from the committee could comment?

@aganea
Copy link
Member

aganea commented Oct 3, 2023

Digging a bit more into this, it is https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html that changed the wording for 28.4.1/2/. From what I understand, that allows for "implementations", that is the compiler and the std library to provide floating-point types, but not user code, like here.

@iburyl since you're one of the authors, would have time to comment please? I feel the MS-STL warning is a bit restrictive right now, in the absence of a proper implementation for P1467, [1] and [2]. But in the end there seems to be now way to provide user floating-point types, like APFloat here. What would be the best avenue here? I mean on the short term we could disable the warning, but what about on the long term?

[1] https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033/32
[2] https://reviews.llvm.org/D149573

@aganea
Copy link
Member

aganea commented Oct 3, 2023

@dkolsen-pgi @griwes

@dkolsen-pgi
Copy link
Contributor

dkolsen-pgi commented Oct 3, 2023

p1467r9 didn't change the wording in any meaningful way. std::complex had always been restricted to the floating-point types that are built into the compiler. p1467r9 changed which floating-point types can be built into the compiler, so we had to tweak the wording for std::complex. But it still has the same meaning. std::complex<APFloat> has never been standards conforming, and there is no effort afoot that I know of to make it standards conforming.

I don't have an opinion on whether it is better to silence the warning, or to change the code to not use std::complex<APFloat>, or to convince Microsoft to get rid of the warning and support std::complex on user-defined types as an extension. I don't have enough information to make an informed decision about that.

@iburyl
Copy link

iburyl commented Oct 4, 2023

Agree with @dkolsen-pgi. From the standard perspective std::complex<APFloat> is not a conforming instantiation. From p1467r9 perspective APFloat is not a conforming extended floating-point type for a number of reasons, thus std::complex<APFloat> will continue be non-conforming.

From my perspective Warning is here in the right place and says the right thing - there is not way to guarantee correctness of implementation of such functions as generic header-based complex acos for arbitaray user defined floating-point type (and even less so for integer types).

If there is a believe that APFloat is actually working in those functions, and there is a strong need to keep it working even if it is non-conforming, it is better be enabled individually, rather than removing this Warning for all types.

@benvanik
Copy link
Contributor

benvanik commented Jan 8, 2024

Do we know how wide the usage of the these std::complex-on-AP*-types is/how hard it'd be to remove the offending uses? A few hundred warnings (one for each file including BuiltinAttributes.h) isn't super fun in a clean LLVM build with default compiler settings. We've added the _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING flag in our own project but it's not a great long-term state for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality mlir:core MLIR Core Infrastructure
Projects
None yet
Development

No branches or pull requests

9 participants