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
Compiling LLVM 14.x with clang-cl
and /arch:AVX
or -march=sandybridge
crashes.
#54645
Comments
@llvm/issue-subscribers-clang-codegen |
@llvm/issue-subscribers-clang-driver |
clang-cl
and /arch:AVX
crashes - but -march=skylake
works?clang-cl
and /arch:AVX
or -march=sandybridge
crashes.
Turning off LTO and we get a specific file that crashes:
|
I'll try to reduce it tomorrow. |
Ok this issue is a bit stranger than I originally thought. @sylvain-audi was able to reduce the test case to the following:
This doesn't trigger a crash on Linux with clang - but it happens on Windows. But ONLY if the clang in question is built with MSVC and @RKSimon any idea why this commit could lead to bad things happening in MSVC? I have a case where it reproduces 100% so if you need me to extract anymore data from it I can. |
Can now confirm that reverting this commit from |
I have no good ideas tbh - what machine are you running it on? One (very weak) hunch - in the patch could you try replacing the DWordClearOps variable |
Also, can you tell if its an non-aligned memory access? illegal instruction? |
Your hunch was correct
This makes the crash go away. |
CCing @silvasean who added the |
Which version of MSVC? |
I tested both 2019 16.10.11 and 2022 17.1.0. |
|
I'll give this a try tomorrow. |
Do we want to propose the |
I think we need a fix for 14.0.1 at least. But I'll defer to you guys what the easiest fix is. |
Also, I'll mention that |
I printed the value from When compiling without Are we using SmallVector with the default value in many places? Then I think we need a fix for CalculateSmallVectorDefaultInlinedElements - otherwise we can probably just roll back this one. |
This could very well be an issue with alignment indeed, but that may be a bug in MSVC. It seems worth reporting the reduced test case to Microsoft. Does it reproduce with the most recent MSVC available? I don't think a fix in |
Ok figured out how to catch this in the debugger - it crashes here:
The actual crash is:
Dissembling this part shows this (sorry for the screenshot - but I couldn't figure out how to copy and paste from the disasm view in vscode): |
Do we want to merge this to the release/14.x branch or are we waiting for a better fix? |
The current fix LGTM for release branch. I think this particular code should have an explicit small size anyway since it's using fixed-size data. I think the ongoing discussion is just to figure out why it's necessary, to understand what issues might be buried elsewhere. |
If at all possible it'd be great to see msvc /AVX and /AVX2 2-stage buildbots added so that we can start tracking this. @sstamenova @rnk Are either of you to add additional bots? |
…to avoid MSVC AVX alignment bug As discussed on Issue llvm#54645 - building llc with /AVX can result in incorrectly aligned structs (cherry picked from commit cb5c4a5)
I can't speak for Reid, but we're not able to right now. We're toying with the idea of maybe adding another |
Merged: ec13fed |
Did somebody find a root cause? If not, then this issue should be still in open state, @tstellar . |
Alternatively we prevent MSVC /arch:AVX builds through cmake. |
Maybe we should at least warn it's not a tested setup. |
But there are maaany possible setups which are not tested. Reverse it? Maybe there should be a list of “tested” setups (configurations) and anything else is possibly broken and may not work. |
I think that's a bigger discussion - there is probably a lot of people building "unsupported" configurations. I think it makes sense to give warnings when people try to do a configuration that is known to have problems. Just listing and detecting the "supported" configurations is still a huge list. I suggest we detect MSVC + /arch:AVX and say |
I'm removing the Release Milestone, since we've already merged a fix to the release branch and there still seems to be an outstanding issue in main. |
Would we be better off splitting this into new tickets and resolving this one?
|
I posted a diff for the CMake warning here: https://reviews.llvm.org/D123777 |
Add a new CMake file to expand on for more problematic configurations in the future. Related to #54645 Reviewed By: beanz, phosek, smeenai Differential Revision: https://reviews.llvm.org/D123777
Warning has landed. I will close this issue now, feel free to re-open if you think there is more we can do here. Thanks everyone helping out! |
@tru I get a:
Compiling with:
|
Sounds like a compiler setup problem to me. You should check what the CMake logs say. If compiler ID is not set it means CMake failed to find the compiler or can't recognise it. |
If you have problems with it still and think it's a bug in LLVMs code please file a new issue so we can have the discussion there instead. |
Setup:
cmake -GNinja -DLLVM_ENABLE_LTO=Thin -DCMAKE_CXX_FLAGS="/arch:AVX" -DCMAKE_C_FLAGS="/arch:AVX" -DCMAKE_C_COMPILER=clang-cl.exe -DCMAKE_CXX_COMPILER=clang-cl.exe -DCMAKE_LINKER=lld-link.exe ..\llvm
llvm-profdata
(it probably happens with other binaries - but this is pretty fast and easy to reproduce it with)crash in:
I have also tested with
-march=sandbridge
which equals crash and-march=skylake
which works.The text was updated successfully, but these errors were encountered: