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

Update to LLVM 13.0.0 #3837

Merged
merged 6 commits into from
Oct 2, 2021
Merged

Update to LLVM 13.0.0 #3837

merged 6 commits into from
Oct 2, 2021

Conversation

chalcolith
Copy link
Member

Updated our LLVM submodule to llvm-org-13.0.0 and updated code to compile with it.

@chalcolith chalcolith added do not merge This PR should not be merged at this time changelog - changed Automatically add "Changed" CHANGELOG entry on merge labels Sep 1, 2021
@ponylang-main
Copy link
Contributor

Hi @kulibali,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3837.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@chalcolith
Copy link
Member Author

This should wait for the official release.

No API changes at all, only the JIT needed a couple of things.

@SeanTAllen
Copy link
Member

Awesome.

Do you know when the release is? Will the commit change then?

@chalcolith
Copy link
Member Author

RC3 is scheduled for September 7 (https://lists.llvm.org/pipermail/llvm-dev/2021-August/152353.html); they usually mark it final a few days after RC3. When there is an official tag we'll need to update the submodule again in this branch.

@chalcolith
Copy link
Member Author

Obligatory "It works fine on my Ubuntu 20.04 machine" post. I'll investigate further.

@SeanTAllen
Copy link
Member

That's a cool error though.

@jemc
Copy link
Member

jemc commented Sep 8, 2021

Obligatory "It works fine on my Fedora 31 machine" post.

I wasn't able to reproduce the CI error.

I don't know much about the CI setup for LLVM building, but is it possible there is a CI cache issue?

@SeanTAllen
Copy link
Member

The change to CMakeLists.txt would bust the cache.

You can see from the runs that the cache was rebuilt:

image

@SeanTAllen
Copy link
Member

I'm going to try locally to see what I get for results.

@SeanTAllen
Copy link
Member

SeanTAllen commented Sep 8, 2021

Using an Ubuntu 20.04 system with WSL2, I am able to recreate the issue from CI:

Optimising
Verifying
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Error:
Module verification failed: Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings
Intrinsic name not mangled correctly for type arguments! Should be: llvm.masked.gather.v4p0s_s.v4p0p0s_s.0
<4 x %9929*> (<4 x %9929**>, i32, <4 x i1>, <4 x %9929*>)* @llvm.masked.gather.v4p0s_Strings.v4p0p0s_Strings

I used the following commands to build (same as CI):

make libs build_flags=-j8
make configure arch=x86-64
make build arch=x86-64 build_flags=-j8
make test-ci arch=x86-64

My clang version is clang version 10.0.0-4ubuntu1.

doing:

make libs build_flags=-j8
make configure
make build
make test-ci

also fails.

note in both cases, linking for debug works, it fails when doing optimized builds.

@SeanTAllen
Copy link
Member

SeanTAllen commented Sep 8, 2021

@kulibali are you sure you checked everything in and pushed it all?

the other question would be... did you try doing make test-ci locally that runs both debug and optimized versions of the test suite? or were you only doing debug versions?

@jemc
Copy link
Member

jemc commented Sep 8, 2021

make test-ci works for me locally

@SeanTAllen
Copy link
Member

Turning off the inliner on line 1381 of genopt.cc fixes the issue for me, so it would appear to be related to name mangling of inlined functions.

    //pmb.Inliner = createFunctionInliningPass(275);

@SeanTAllen
Copy link
Member

@jemc can you put the command you used to build here? is there a difference from either of mine?

@jemc
Copy link
Member

jemc commented Sep 8, 2021

The only difference is that I used 4 instead of 8 cores to build LLVM, so it wouldn't bring my system to a crawl:

make libs build_flags=-j4
make configure
make build
make test-ci

@jemc
Copy link
Member

jemc commented Sep 8, 2021

Looks like this is an issue with the LLVM vectorization optimizations - maybe my CPU is different enough from yours that LLVM is trying to make different vectorization choices?

@SeanTAllen
Copy link
Member

maybe my CPU is different enough from yours that LLVM is trying to make different vectorization choices?

I have an I9 vPro.

@jemc
Copy link
Member

jemc commented Sep 8, 2021

@SeanTAllen - does it compile successfully for you if you disable Pony-specific optimizations?

That would hopefully indicate whether it may be a bug in our Pony-specific optimizations or a bug in LLVM itself.

@SeanTAllen
Copy link
Member

@jemc

does it compile successfully for you if you disable Pony-specific optimizations?

Nope. That's the first thing I tried.

@jemc
Copy link
Member

jemc commented Sep 8, 2021

My CPU on this machine is:

Model name:                      Intel(R) Core(TM) i7-3740QM CPU @ 2.70GHz

@chalcolith
Copy link
Member Author

Mine is Model name: AMD Ryzen 7 3700X 8-Core Processor

@jemc
Copy link
Member

jemc commented Sep 8, 2021

This may be a legitimate LLVM bug that we may want to consider reporting as part of their release candidate validation process.

@SeanTAllen
Copy link
Member

@kulibali is there still time to report a bug on the 13.00 release?

@chalcolith
Copy link
Member Author

Not sure. I only have superficial knowledge of the release process. I'm in a bit of a crunch at work today so if you have time to investigate that would be great.

@SeanTAllen
Copy link
Member

@kulibali i have a deadline for the 15th so, I don't have time to look into it. I had a bit of downtime this morning while waiting on others that I used to do the triaging.

@SeanTAllen
Copy link
Member

Turning off pmb.populateLTOPassManager(lpm) while leaving the inliner on also works.

@SeanTAllen
Copy link
Member

Turning off lpm.add(createStripSymbolsPass()); also results in a build that works.

@SeanTAllen
Copy link
Member

SeanTAllen commented Sep 9, 2021

I dont understand how the pass manager works so this is probably wrong but... moving

pmb.populateLTOPassManager(lpm); to the last thing done in optimise also works. but i really dont understand what orderings are important and how the pmb relates to the other passes.

Would I be correct in assuming that effectively turns the pass off?

@SeanTAllen
Copy link
Member

So I think this might be our best fix:

@kulibali @jemc

changing to:

createStripSymbolsPass(true)

which ONLY strips debug symbols. also works for me.

Here's the definition:

// These functions removes symbols from functions and modules.  If OnlyDebugInfo
// is true, only debugging information is removed from the module.
//
ModulePass *createStripSymbolsPass(bool OnlyDebugInfo = false);

@SeanTAllen
Copy link
Member

@jemc how do you feel about the fix?

@jemc
Copy link
Member

jemc commented Sep 9, 2021

Sounds good to me - I gave my 👍 emoji earlier on your comment above.

@SeanTAllen
Copy link
Member

I never ever notice emojis @jemc as I get no notification from GitHub about them.

@SeanTAllen
Copy link
Member

I just noticed these emojis @jemc...

image

@chalcolith chalcolith removed the do not merge This PR should not be merged at this time label Oct 2, 2021
@chalcolith
Copy link
Member Author

RC4 has now been tagged as llvmorg-13.0.0.

@SeanTAllen
Copy link
Member

Awesome.

@SeanTAllen
Copy link
Member

@kulibali so we can merge now, yes?

@chalcolith
Copy link
Member Author

Yes.

@SeanTAllen SeanTAllen merged commit 20613d9 into main Oct 2, 2021
@SeanTAllen SeanTAllen deleted the llvm1300 branch October 2, 2021 02:55
github-actions bot pushed a commit that referenced this pull request Oct 2, 2021
github-actions bot pushed a commit that referenced this pull request Oct 2, 2021
@SeanTAllen
Copy link
Member

Ugh I forgot to squash so this went in as 6 commits. Ooops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants