-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
A precompilation error on the latest assertion build #45302
Comments
It seems like this doesn't happen on Linux? I couldn't reproduce it on:
|
@gbaraldi I suspect this may be related to the allocation hoisting change. Could you take a look? |
Having written something similar to |
@aviatesk what OS did you use to get the bug? Because on my mac m1 I just works 🤔 |
I don't have the access to my mac atm, but it's not using M1 tip at least. |
It doesn't look to be the allocation hoisting pr. Will bisect |
The error is probably in VectorizationBase.jl here, where there are 3 typ in a list instead of 2: https://github.com/JuliaSIMD/VectorizationBase.jl/blob/9ae47c5c12c7bec9b57c132c24f67c4f14090c9e/src/llvm_intrin/masks.jl#L336 |
Why doesn't the IR verifier catch that? |
The IR verification currently runs in a few optimization passes only, and furthermore cannot catch every possible IR error (I've seen mismatched context errors on attributes escape detection for a while). I haven't added it to codegen because I'm not sure it produces 100% valid IR until after all modules have been merged due to globals and such (more likely I haven't figured out the validity point). Since this error happens during module merging, I imagine it would be difficult to detect unless we added a verification step prior to module merge, which would probably cause a large slowdown in debug builds due to number of modules. |
Fair enough. I thought this was in codegen, but if it's during module merging that makes sense. |
I wonder if we should run pkgeval with assertions on. This commit was out for quite a while before this was found. Not sure how much it would add to the runtime of the test but it would've caught this a lot earlier. |
FYI, that's already possible by specifying buildflags to the @nanosoldier invocation. See https://github.com/JuliaCI/Nanosoldier.jl#trigger-syntax-1= |
@chriselrod can you fix this problem on LoopVectorization.jl side? |
Note that This likely explains difficulties reproducing the problem, e.g. on znver2. |
Given the difficulty for the verifier to catch this error, should we close this? Or do we want to discuss whether we should enable the assertions by default on nanosoldier? |
For this particular case, couldn't we have called the intrinsic with ccall and the llvmcall calling convention? That might have allowed us to do better validation that the number of arguments is correct. |
Close this since the original issue has beee resolved. |
Especially I found a precompilation on
LoopVectorization
fails on the latest assertion build with the following error:The error disappears if we build without the assertion flags.
The text was updated successfully, but these errors were encountered: