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

Avoid a warning in clang that IS_64BIT is already defined #4485

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

Conversation

maximmasiutin
Copy link
Contributor

@maximmasiutin maximmasiutin commented Apr 1, 2023

We avoid a warning in clang (LLVM) compiler that IS_64BIT is already defined by checking whether IS_64BIT was defined earlier. If it was already defined, we do not define it again, thus avoiding the warning.

To reproduce it, run make.exe build ARCH=x86-64-vnni512 COMP=clang or make.exe build ARCH=x86-64-vnni512 COMP=icx under Windows. Prerequisite: LLVM Windows binary, Cygwin make.

…ined by checking whether it was defined earlier, before defining it
@vondele
Copy link
Member

vondele commented Apr 1, 2023

The comment above says // No Makefile used and so assumes that if _MSC_VER is defined no Makefile is used. I don't think silencing of warnings this way is helpful. The question why is there a warning, what's the assumption that is wrong, and that can be addressed or not.

@maximmasiutin
Copy link
Contributor Author

The Makefile is used. I run the command make.exe build ARCH=x86-64-vnni512 COMP=clang under Windows ang get this warning. I use native Windows binary for clang.exe and CygWin makefile. The same error happens in COMP=icx for Windows. If you think that we should modify the Makefile rather than the .h file, let me know. The Clang compiler (and the icx) define _MSC_VER for compatibility.

@maximmasiutin
Copy link
Contributor Author

maximmasiutin commented Apr 1, 2023

This is how clang is called:

clang++  -Wall -Wcast-qual -fno-exceptions -std=c++17 -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_AVX2 -mavx -mavx2 -mbmi -DUSE_AVX512 -mavx512f -mavx512bw -DUSE_VNNI -mavx512f -mavx512bw -mavx512vnni -mavx512dq -mavx512vl -mprefer-vector-width=512 -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -DUSE_PEXT -mbmi2 -DGIT_SHA=6e830117 -DGIT_DATE=20230401 -c -o benchmark.o benchmark.cpp

As you see, make somehow adds the -DIS_64BIT command line parameter for some reason. Do you think that we should remove it?

@vondele
Copy link
Member

vondele commented Apr 1, 2023

No, the problem is we're trying to have some code trying to support compiling without Makefile / supporting MSCV, other compilers trying to behave like MSCV while using Makefile.

@maximmasiutin
Copy link
Contributor Author

What if we define something like GENUINE_MSVC in an appropriate .h file that will be only defined if _MSC_VER is defined but not __llvm__ or __clang__ or Intel's defines are defined, so that we will avoid clashes like we had with incbin and with that one? It will also resolve displaying proper compiler name. I had such code but didn't create a pull request for that since there were already too many pull requests.

@snicolet
Copy link
Member

Can we instead modify line 66 to read like that?

   #if defined(_WIN64) && defined(_MSC_VER) && !defined(__clang__)  // No Makefile used

@snicolet
Copy link
Member

snicolet commented Sep 5, 2023

@maximmasiutin what would you think about the above solution? :-)

@maximmasiutin
Copy link
Contributor Author

I am OK with any solution as long as there will be no warning in clang that IS_64BIT is already defined.

@snicolet
Copy link
Member

snicolet commented Sep 5, 2023

Can you confirm that this would fix the warning for you? :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants