Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upbump minimum LLVM version to 5.0 #51899
Conversation
rust-highfive
assigned
alexcrichton
Jun 29, 2018
rust-highfive
added
the
S-waiting-on-review
label
Jun 29, 2018
gnzlbg
reviewed
Jun 29, 2018
| @@ -666,11 +666,7 @@ impl<'a, 'tcx> FnTypeExt<'a, 'tcx> for FnType<'tcx, Ty<'tcx>> { | |||
| layout::Int(..) if !scalar.is_bool() => { | |||
| let range = scalar.valid_range_exclusive(bx.cx); | |||
| if range.start != range.end { | |||
| // FIXME(nox): This causes very weird type errors about | |||
| // SHL operators in constants in stage 2 with LLVM 3.9. | |||
This comment has been minimized.
This comment has been minimized.
gnzlbg
reviewed
Jun 29, 2018
| @@ -14,7 +14,7 @@ | |||
| // ignore-tidy-linelength | |||
| // ignore-windows | |||
| // ignore-macos | |||
| // min-system-llvm-version 5.1 | |||
| // min-llvm-version 6.0 | |||
This comment has been minimized.
This comment has been minimized.
gnzlbg
Jun 29, 2018
Author
Contributor
Oops, I don't know if I messed this up. Like LLVM 5.1 does not exist, are we sure whatever change was needed was not part of LLVM 6.0 and that the patch will be backported to LLVM 5.1 if it is ever released (which seems unlikely) ?
This comment has been minimized.
This comment has been minimized.
cuviper
Jun 29, 2018
Member
That was part of #45897, which included rust-lang/llvm#97 with D39503. That patch is indeed included in LLVM 6.0, and I don't think there's any point tagging this for a hypothetical 5.1.
gnzlbg
reviewed
Jun 29, 2018
| @@ -8,9 +8,6 @@ | |||
| // option. This file may not be copied, modified, or distributed | |||
| // except according to those terms. | |||
|
|
|||
| // asmjs can't even pass i128 as arguments or return values, so ignore it. | |||
| // this will hopefully be fixed by the LLVM 5 upgrade (#43370) | |||
| // ignore-asmjs | |||
This comment has been minimized.
This comment has been minimized.
gnzlbg
Jun 29, 2018
Author
Contributor
The issue is closed, we'll se if this works, but if it doesn't, it might need a new issue.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 29, 2018
Member
Since asmjs isn't upgraded here this is probably still an issue but I'm fine waiting for bors too!
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
r=me with tests passing, nice cleanup! |
cuviper
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Jun 29, 2018
cuviper
reviewed
Jun 29, 2018
| let mut variable_access = VariableAccess::DirectVariable { | ||
| // byval attribute, for which LLVM always does the deref itself, | ||
| // so we must not add it. | ||
| let variable_access = VariableAccess::DirectVariable { |
This comment has been minimized.
This comment has been minimized.
cuviper
Jun 29, 2018
Member
So, this sort of change is why I pointed out that llvm-emscripten is still on LLVM 4. Won't it be broken by this?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jun 29, 2018
Member
If Emscripten had working debuginfo it might be broken yeah but I don't think it has debuginfo to nearly this level of fidelity for it to matter too much
This comment has been minimized.
This comment has been minimized.
|
I can't reproduce the failure locally, so I'd guess this is a regression in the LLVM 5 version being used here (is there an easy way to print which exact LLVM version is used in CI?). I've bumped the minimum llvm version of the failing test to LLVM 6, but maybe the solution here would be to make sure that we use LLVM 5.0.1 or 5.0.2 in CI. |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton can we try this? |
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ |
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
Jul 2, 2018
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jul 3, 2018
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-review
and removed
S-waiting-on-bors
labels
Jul 3, 2018
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Jul 4, 2018
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton so emscripten didn't like this. Should I revert the changes that emscripten did not like, or should I do something else? |
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg yeah the RustWrapper.cpp and such files will still need to compile on LLVM 4.0 for Emscripten, |
This comment has been minimized.
This comment has been minimized.
|
|
gnzlbg
added some commits
Jun 29, 2018
gnzlbg
force-pushed the
gnzlbg:llvm501
branch
from
2fe19fa
to
fe75f61
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
gnzlbg
force-pushed the
gnzlbg:llvm501
branch
from
5e2e1a9
to
3b36ce6
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton this should be fixed |
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ |
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
|
|
gnzlbg commentedJun 29, 2018
•
edited
Closes #51878 .
r? @alexcrichton
--
cc @cuviper @infinity0