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

Backports for julia-1.6.3 #41554

Merged
merged 73 commits into from
Sep 7, 2021
Merged

Backports for julia-1.6.3 #41554

merged 73 commits into from
Sep 7, 2021

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jul 12, 2021

Backported PRs:

Non-merged PRs with backport label:

@KristofferC KristofferC added the release Release management and versioning. label Jul 12, 2021
@simeonschaub
Copy link
Member

If we are backporting #39618, we will also have to backport #39709, since it fixes a bug introduced in that PR.

@KristofferC
Copy link
Member Author

Thanks, I backported #39618 because it was tangled up with #39980 which fixed a bug (#41096).

@KristofferC KristofferC force-pushed the backports-release-1.6 branch 2 times, most recently from e133d5c to 36169a9 Compare July 19, 2021 09:02
@vchuravy
Copy link
Member

cc: @tkf @staticfloat @DilumAluthge the Asan toolchain seems to be broken here, can someone take a look?

@KristofferC
Copy link
Member Author

@vchuravy, #41606 I guess.

@vchuravy
Copy link
Member

@vchuravy, #41606 I guess.

Feels more like something is wrong with the setup script since it is complaining that it can't find a certain version of libLLVM.

@tkf
Copy link
Member

tkf commented Jul 19, 2021

#41530 requires libblastrampoline but 1.6 doesn't have it. So, I guess it's actually not appropriate for backport? Not sure if it's related to the error per se, though.

@DilumAluthge
Copy link
Member

DilumAluthge commented Jul 20, 2021

I would just go ahead and backport all of the CI PRs. They tend to build on one another, and I think it will be easiest to just backport them all.

One of the things that PR #41606 (which is labeled for backport) does is that it only runs asan on master. So if you backport #41606 to this branch, asan will no longer be run on release-1.6.

@DilumAluthge
Copy link
Member

Alternatively, if you don't backport #41530 to 1.6, that's also completely fine. There's no problem if the Buildkite configuration files on release-1.6 differ from those on master, since one of the benefits of Buildkite is that we can maintain independent pipelines for master, release-1.6, release-1.7, etc.

So either way is fine (backport #41530 or don't backport #41530).

@KristofferC
Copy link
Member Author

@nanosoldier runtests(ALL, vs=":release-1.6")

@KristofferC KristofferC force-pushed the backports-release-1.6 branch from 0fb9425 to 7ecd8dd Compare July 26, 2021 11:36
@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

@nanosoldier runtests(["AKNS", "BayesianLinearRegressors", "BetterFileWatching", "BioStructures", "BoltzmannMachines", "Brillouin", "BytePairEncoding", "CalibrationErrorsDistributions", "ChaosTools", "ComplexMixtures", "CountTimeSeries", "DeepForest", "DiffEqParamEstim", "DistanceTransforms", "ElectricFields", "Enzyme", "FilesystemDatastructures", "FuzzyCompletions", "Glowe", "HePPCAT", "ITensors", "ImGuiOpenGLBackend", "IntervalRootFinding", "JET", "Khepri", "KissMCMC", "LowLevelParticleFilters", "ManifoldsBase", "MatrixProfile", "NumericalAlgorithms", "PrincipalMomentAnalysisApp", "PushVectors", "RateLimiter", "ReferenceTests", "ReverseDiff", "SatelliteDynamics", "SpatialJackknife", "StanOptimize", "StochasticDelayDiffEq", "SuiteSparseMatrixCollection", "Symbolics", "Tectonic", "ZigZagBoomerang"], vs = ":release-1.6")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

Jeez, that's a lot of CI commits.

@DilumAluthge
Copy link
Member

DilumAluthge commented Aug 11, 2021

Yeah, as we move to Buildkite, all of the config will live in the Julia repo (in contrast to Buildbot, in which the config lived in an external repo).

There will continue to be a high volume of Buildkite commits for a while, but I do expect that once the migration is completely done, the volume will slow down.

I'll work on fixing the broken llvmpasses job.

@vtjnash
Copy link
Member

vtjnash commented Aug 24, 2021

#41774 just needs to be cherry-picked here, since it is already the backport commit

@KristofferC
Copy link
Member Author

@DilumAluthge any idea why the CI commits fail to backport? Are we missing a label on something that then causes merge conflicts?

@DilumAluthge
Copy link
Member

Are we missing a label on something that then causes merge conflicts?

That seems like the most likely culprit.

@DilumAluthge
Copy link
Member

DilumAluthge commented Aug 25, 2021

Hmmm, actually....

Are you using a GitHub personal access token to do these pushes?

If so, can you check what permissions that token has?

@DilumAluthge
Copy link
Member

Are we missing a label on something that then causes merge conflicts?

That seems like the most likely culprit.

Okay, I think I found the missing PRs - they were the PRs that modified the CODEOWNERS file. I've added the backport label to all of those PRs.

@KristofferC
Copy link
Member Author

Great, that made the bunch of them go through.

@KristofferC
Copy link
Member Author

@nanosoldier runtests(ALL, vs=":release-1.6")

Keno and others added 6 commits September 3, 2021 12:20
Recall the reproducer from the issue:
```
julia> f() = (if nothing; end; unsafe_load(Ptr{Int}(0)))
f (generic function with 1 method)

julia> f()
Unreachable reached at 0x7fb33bb50090

signal (4): Illegal instruction
in expression starting at REPL[13]:1
unsafe_load at ./pointer.jl:105 [inlined]
unsafe_load at ./pointer.jl:105 [inlined]
```

There were actually two places where we were dropping the
GotoIfNot, one in type annotation after inference, one in
SSA conversion. The one in SSA conversion was structural:
When both branches target the same jump destination, the
GotoIfNot would be dropped. This was fine in general, except
that as shown above, GotoIfNot can actually itself have
a side effect, namely throwing a type error if the condition
is not a boolean. Thus in order to actually drop the node
we need to prove that the error check does not fire.

The reason we want to drop the GotoIfNot node here is
that IRCode has an invariant that every basic block is
in the predecessor list only once (otherwise PhiNodes
would have to carry extra state regarding which branch
they refer to).

To fix this, insert an `Expr(:call, typecheck, _, Bool)`
when dropping the GotoIfNot. We do lose the ability to
distinguish the GotoIfNot from literal typechecks as
a result, but at the moment they generate identical
errors. If we ever wanted to dinstinguish them, we could
create another typecheck intrinsic that throws a different
error or use an approach like #41994.

(cherry picked from commit 2445000)
When CFunction closures are created an extra argument is added to the
function signature for holding the closure.

Make sure that the parameter attributes on already existing parameters
are not shifted when adding that parameter.

(cherry picked from commit 08f3422)
If the first loop exits in the first iteration, the `statetype` is still
`Bottom`. In that case, the new `stateordonet` needs to be determined
with the two-arg version of `iterate` again.

Explicitly test that inference produces a sound (and reasonably precise)
result when splatting an iterator (in this case a long range) that
allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit.

Fixes #41022

Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 92337b5)
We might previously accidentally visit this use after deletion, if the
orig_inst ended up back in the workqueue.

Fixes #41916

(cherry picked from commit d8a8db2)
(cherry picked from commit 4598966)
@KristofferC KristofferC force-pushed the backports-release-1.6 branch from 7b28c71 to 3c2ee37 Compare September 3, 2021 10:20
@KristofferC
Copy link
Member Author

@nanosoldier runtests(ALL, vs=":release-1.6")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

@nanosoldier runtests(["AlphaStableDistributions", "ApproxFunBase", "BLASBenchmarksCPU", "BitInformation", "CodeTransformation", "ConstraintModels", "CopEnt", "Earth2014", "ElasticArrays", "EliminateGraphs", "JET", "RateLimiter", "SIMD", "StochasticRounding", "StrideArraysCore", "VideoIO", "ZigZagBoomerang"], vs = ":release-1.6")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

KristofferC commented Sep 5, 2021

SIMD is just a consequence of #41510 and the SIMD tests should be fixed.

ApproxFunBase should be looked at. Edit: Bisected to #41510

StochasticRounding passes locally

StrideArraysCore should be looked at. Edit: Also #41510.

VideoIO @test_broken -> working.

Conclusion, dropping #41510 from backports.

vtjnash and others added 7 commits September 6, 2021 14:56
* InteractiveUtils: recursive correctly in varinfo, et al.

Fixes #42045

(cherry picked from commit a163e37)
Same bug as 5e57c21 (#26833), same fix.

(cherry picked from commit 82c4a27)
Create THIRDPARTY.md to hold license information for all code not covered by the main MIT license. This format allows for automated scanning and categorization of Julia's license.  The licenses were broken up this way because this is the format that many automated license scanners (including GitHub).

(cherry picked from commit 161e384)
If the unionall bounds are inconsistent with the wrapper's bound, avoid
throwing due to an impossible type instantiation.

(cherry picked from commit b5b0684)
We will not use the duplicate, so best to try to avoid loading it.

(cherry picked from commit c53669f)
@KristofferC KristofferC force-pushed the backports-release-1.6 branch from 4651333 to 19e66b3 Compare September 6, 2021 12:56
@KristofferC
Copy link
Member Author

@nanosoldier runtests(["AlphaStableDistributions", "ApproxFunBase", "BLASBenchmarksCPU", "BitInformation", "CodeTransformation", "ConstraintModels", "CopEnt", "Earth2014", "ElasticArrays", "EliminateGraphs", "JET", "RateLimiter", "SIMD", "StochasticRounding", "StrideArraysCore", "VideoIO", "ZigZagBoomerang"], vs = ":release-1.6")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@KristofferC
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":release-1.6")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(setenv(`git push`; dir="/run/media/system/data/nanosoldier/workdir/NanosoldierReports"), ProcessExited(1)) [1]

Unfortunately, the logs could not be uploaded.
cc @christopher-dG

@vtjnash
Copy link
Member

vtjnash commented Sep 7, 2021

  From worker 2:    remote: error: See http://git.io/iEPt8g for more information.        
  From worker 2:    remote: error: File benchmark/by_hash/cd5b8fb_vs_7c45ff0/data.tar.xz is 107.50 MB; this exceeds GitHub's file size limit of 100.00 MB        
  From worker 2:    remote: error: GH001: Large files detected. You may want to try Git Large File Storage - https://git-lfs.github.com.        
  From worker 2:    To https://github.com/JuliaCI/NanosoldierReports.git
  From worker 2:     ! [remote rejected]         master -> master (pre-receive hook declined)

I've uploaded the report manually (without the data file) to
https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/cd5b8fb_vs_7c45ff0/report.md

@KristofferC KristofferC merged commit 11e64d1 into release-1.6 Sep 7, 2021
@KristofferC KristofferC deleted the backports-release-1.6 branch September 7, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Release management and versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.