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

Add support for LLVM 11 #3546

Merged
merged 17 commits into from
Sep 30, 2020
Merged

Add support for LLVM 11 #3546

merged 17 commits into from
Sep 30, 2020

Conversation

kinke
Copy link
Member

@kinke kinke commented Aug 23, 2020

Quite a PITA this time.

if (MCPU == "help")
return true;
return std::any_of(MAttrs.begin(), MAttrs.end(),
[](const std::string &a) { return a == "help"; });
}

TargetOptions InitTargetOptionsFromCodeGenFlags() {
#if LDC_LLVM_VER >= 1100
return codegen::InitTargetOptionsFromCodeGenFlags();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you already did all the work, but maybe a using codegen would've worked well too.

@JohanEngelen
Copy link
Member

oof indeed. Nice work

@kinke kinke force-pushed the llvm11 branch 14 times, most recently from 69909c7 to 181bf39 Compare August 25, 2020 23:21
@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

Should be finally almost ready, just a couple of test regressions wrt. optimized code (std.bitmanip for Mac & Linux x64, 2 other failures for Win32) + failing instrument/xray_link.d for Mac. I guess the necessary adaptations would have been even more painful when still supporting LLVM 3.9...

@kinke kinke marked this pull request as ready for review August 26, 2020 01:06
@JohanEngelen
Copy link
Member

JohanEngelen commented Aug 26, 2020

I'm working on the macOS Xray failure right now.

Edit: Clang trunk has the same problem. llvm/llvm-test-suite@2c3c4a6
So we should disable this test for now.

@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

I've had a look at the std.bitmanip issue (FWIW, on Linux). It looks pretty insane:

import std.bitmanip : bitsSet;
import std.algorithm.comparison : equal;

void main()
{
    auto a = bitsSet(3);
    assert(a.length == 2);
    int[2] expected = [0, 1];
    assert(a.equal(expected[]));
}
$ ldc2 -O -run bla.d
core.exception.AssertError@/home/martin/ldc/runtime/phobos/std/range/primitives.d(2432): Attempting to fetch the front of an empty array of int

It works fine when making the expected array static immutable.

The optimized .ll is incredible - no traces of the bitsSet left:

define i32 @_Dmain({ i64, { i64, i8* }* } %unnamed) #0 {
assertPassed:
  ; 1) allocate and store `expected` array
  %expected = alloca [2 x i32], align 4           ; [#uses = 3, size/byte = 8]
  %.fca.0.gep = getelementptr inbounds [2 x i32], [2 x i32]* %expected, i64 0, i64 0 ; [#uses = 2, type = i32*]
  store i32 0, i32* %.fca.0.gep, align 4
  %.fca.1.gep = getelementptr inbounds [2 x i32], [2 x i32]* %expected, i64 0, i64 1 ; [#uses = 1, type = i32*]
  store i32 1, i32* %.fca.1.gep, align 4
  ; 2) load the 1st elem and assert it's 0 - it's just been stored!
  %0 = load i32, i32* %.fca.0.gep, align 4        ; [#uses = 1]
  %1 = icmp eq i32 %0, 0                          ; [#uses = 1]
  br i1 %1, label %assertPassed.i.i, label %assertFailed2

assertPassed.i.i:                                 ; preds = %assertPassed
  ; 3) load the 2nd elem and assert it's 1, as if it hadn't just been stored...
  %lowerbound.i.i = getelementptr inbounds [2 x i32], [2 x i32]* %expected, i64 0, i64 1 ; [#uses = 1, type = i32*]
  %2 = load i32, i32* %lowerbound.i.i, align 4    ; [#uses = 1]
  %3 = icmp eq i32 %2, 1                          ; [#uses = 1]
  br i1 %3, label %assertPassed.i.i.1, label %assertFailed2

assertPassed1:                                    ; preds = %assertPassed.i.i.1
  ret i32 0

assertFailed2:                                    ; preds = %assertPassed.i.i, %assertPassed
  tail call void @_d_assert({ i64, i8* } { i64 12, i8* getelementptr inbounds ([13 x i8], [13 x i8]* @.str, i32 0, i32 0) }, i32 9) #2
  unreachable

assertPassed.i.i.1:                               ; preds = %assertPassed.i.i
  ; 4) FUN! *undefined* branch, apparently jumping to forbody.i.2 which throws the assertion error...
  br i1 undef, label %assertPassed1, label %forbody.i.2

forbody.i.2:                                      ; preds = %assertPassed.i.i.1
  tail call void @_d_assert_msg({ i64, i8* } { i64 54, i8* getelementptr inbounds ([55 x i8], [55 x i8]* @.str.4, i32 0, i32 0) }, { i64, i8* } { i64 54, i8* getelementptr inbounds ([55 x i8], [55 x i8]* @.str.5, i32 0, i32 0) }, i32 2432) #2
  unreachable
}

@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

It works fine when making the expected array static immutable.

Yeah well, just by accident:

define i32 @_Dmain({ i64, { i64, i8* }* } %unnamed) #0 {
forbody.preheader.i:
  br i1 undef, label %assertPassed1, label %forbody.i.2

assertPassed1:                                    ; preds = %forbody.preheader.i
  ret i32 0

forbody.i.2:                                      ; preds = %forbody.preheader.i
  tail call void @_d_assert_msg({ i64, i8* } { i64 54, i8* getelementptr inbounds ([55 x i8], [55 x i8]* @.str.4, i32 0, i32 0) }, { i64, i8* } { i64 54, i8* getelementptr inbounds ([55 x i8], [55 x i8]* @.str.5, i32 0, i32 0) }, i32 2432) #4
  unreachable
}

@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

With -mtriple=x86_64-pc-windows-msvc, the static immutable thing is optimized to the expected return 0, while the stack variant contains the same poor optimization checking the previous stores, but no undefined branches. So IMO, this has got to be related to EH.

Edit: LLVM 10 on Linux x64 seems to produce equivalent IR as LLVM 11 for Win64, see https://d.godbolt.org/z/nPbYad.

@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

The optimized 32-bit Linux x86 .ll is ugly as hell (incl. the static immutable version), but no undefined branches either. EH-wise, I don't think there should be any relevant divergences...

@kinke
Copy link
Member Author

kinke commented Aug 26, 2020

@kinke
Copy link
Member Author

kinke commented Aug 29, 2020

They are working on it; it's actually an older bug (at least also happening with LLVM 10) which just happened to surface with LLVM 11 for our testcase. The other regression (for Win32) is apparently just some custom NaN not being propagated and can probably be neglected, so yeah, almost ready.

@kinke
Copy link
Member Author

kinke commented Sep 24, 2020

I'm trying to move the AArch64 package generation from Shippable to Travis, as Shippable is constantly timing out again. I need someone to enable the llvm-project repo on Travis though (I've tried setting up a webhook alone, but that doesn't suffice). Maintaining CI would definitely be a lot easier if I could manage the GitHub apps for the ldc-developers organization, but I've already requested those rights multiple times in the last year without a single reply - trying again @dnadlinger @redstar.

@kinke
Copy link
Member Author

kinke commented Sep 24, 2020

Thx to whoever did what they did, it's working now. - Looks like it's too slow unfortunately, plus the SPIR-V translator doesn't compile with gcc 5.4 (and MLIR neither, but that was already the case with IIRC gcc 8 for Shippable, apparently some LLVM 11 regression).

@kinke kinke mentioned this pull request Sep 25, 2020
@kinke
Copy link
Member Author

kinke commented Sep 25, 2020

There's just no way Travis can build LLVM in under 50 mins (I've tried 16-24-32-48-64 threads, ld.bfd and ld.gold, excluding the LLVM tools...) [Shippable takes ~20 mins for the actual build with 64 threads]. According to https://blog.travis-ci.com/2020-09-11-arm-on-aws, migrating to travis-ci.com would allow to experiment with a supposedly much faster AWS Graviton2 CPU. Looks like the domain transition wouldn't come with any real drawbacks, see https://docs.travis-ci.com/user/migrate/open-source-on-travis-ci-com/.

The AArch64 build for Shippable hasn't succeeded (failed to compile
MLIR).
Which supports more cmdline options in the meantime than what we have
been supporting manually, and should be future-proof, similar to
InitTargetOptionsFromCodeGenFlags().
Attributes are only set if explicitly specified in the cmdline (and not
already present in the function's existing attributes).

This started out as a workaround for not being able to determine whether
the user has explicitly set -frame-pointer in the cmdline with LLVM 11,
and ended with having to touch more than I wanted. An *enabled*
-ffast-math flag (from us, not LLVM) overriding LLVM's
-enable-unsafe-fp-math, but e.g. -ffast-math=false NOT overriding was/is
one of the quirks.
LLVM already provides suited RAII helper types to restore the IRBuilder
state. [They sadly aren't movable, so I've had to wrap them in a
unique_ptr.]

While at it, also minimally revise debuginfo generation for functions.
Previously, the static-array alloca wasn't suitably aligned for the
store instruction, which uses the greater vector alignment.
This has surfaced now with LLVM 11 on Win32 - this fixes dmd-testsuite's
runnable/ldc_llvm_inline_ir.d.
@kinke
Copy link
Member Author

kinke commented Sep 26, 2020

Looks like the domain transition wouldn't come with any real drawbacks

Well, encrypting stuff with the (annoyingly, Ruby-based) travis CLI tool needs an extra step, authenticating with a GitHub token, see https://docs.travis-ci.com/user/encryption-keys#usage.


There's an annoying AArch64 regression with LLVM 11 wrt. the llvm_umul... intrinsic (apparently wrongly reporting overflows), only detected yesterday with the first successful LLVM 11 build (see the Travis result).

I'm newly including the AMDGPU backend for the x86 builds; we've included the nvidia one for a while, and excluding AMD seems not really fair, plus we have gccbuiltins support for AMD GCN now too...

@kinke
Copy link
Member Author

kinke commented Sep 29, 2020

AArch64 regression filed as https://bugs.llvm.org/show_bug.cgi?id=47679.

@kinke
Copy link
Member Author

kinke commented Sep 30, 2020

Finally - merging this with non-final LLVM 11 for the prebuilt packages, as the final will most likely not be ready for the beta.

@kinke kinke merged commit d33f6f9 into ldc-developers:master Sep 30, 2020
@kinke kinke deleted the llvm11 branch September 30, 2020 17:36
@jacob-carlborg
Copy link
Contributor

There's just no way Travis can build LLVM in under 50 mins

@kinke FYI, GitHub actions are significantly faster than Travis, in my experience.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Oct 11, 2020

BTW, all tests pass for me on iOS using LLVM 11. Might have been a regression in LLVM 10.

@kinke
Copy link
Member Author

kinke commented Oct 11, 2020

FYI, GitHub actions are significantly faster than Travis, in my experience.

But don't support arm64 AFAIK, the sole reason I've bothered with it (as Shippable isn't reliable anymore).

@jacob-carlborg
Copy link
Contributor

But don't support arm64 AFAIK

Oh, I didn't know Travis support non-x86 architectures. That explains it.

@jacob-carlborg
Copy link
Contributor

But don't support arm64 AFAIK

There's this action: https://github.com/marketplace/actions/run-on-architecture, but that's using emulation.

@kinke
Copy link
Member Author

kinke commented Oct 11, 2020

That's going to be waaay too slow. - Travis.com would be fast enough for LLVM too as stated above (travis.org already is fast enough for LDC itself on native arm64, taking less than 30 mins for a normal CI testrun).


Thx for checking LLVM 11; I'll use it for Bitrise too once the final is out.

@kinke
Copy link
Member Author

kinke commented Oct 13, 2020

Yay, Bitrise is indeed green again for master builds (where the unittests are actually run, as opposed to PRs where they are only compiled).

@jacob-carlborg
Copy link
Contributor

Awesome, let's keep it that way 😀.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants