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 14 #3951

Merged
merged 25 commits into from
Apr 6, 2022
Merged

Add support for LLVM 14 #3951

merged 25 commits into from
Apr 6, 2022

Conversation

kinke
Copy link
Member

@kinke kinke commented Mar 30, 2022

No description provided.

@kinke
Copy link
Member Author

kinke commented Mar 30, 2022

For LLVM 14 itself, I wanted to include BOLT in our prebuilt LLVM packages, just for convenience. The README states:

BOLT operates on X86-64 and AArch64 ELF binaries.

but there's at least some extra runtime library for macOS (causing problems, so I noticed... ldc-developers/llvm-project@0655850) and a source file with MachO in its name, so I moved from Linux-only to Linux and Mac (assuming Windows is not supported). But it apparently requires macOS 10.12+:

/Users/runner/work/1/s/bolt/include/bolt/Core/BinaryContext.h:178:16: error: 'shared_timed_mutex' is unavailable: introduced in macOS 10.12

@JohanEngelen and whoever else is using Mac: would bumping MACOSX_DEPLOYMENT_TARGET (for prebuilt LDC too then) from 10.9 to 10.12 be acceptable/worth it?

@JohanEngelen
Copy link
Member

JohanEngelen commented Mar 31, 2022

I don't know much about macos versions to be honest.
10.12 Sierra was released 6 years ago. https://en.wikipedia.org/wiki/MacOS_version_history#Version_10.12:_%22Sierra%22
I'd say that is acceptable for LDC. Including BOLT would be great.

Edit: my macbook from 2013 is running 11.6, and I am not keen on updating the OS, so 10.12 sounds old even to me.

@kinke
Copy link
Member Author

kinke commented Mar 31, 2022

Thx, 10.12 it'll be then.

@kinke
Copy link
Member Author

kinke commented Apr 2, 2022

Ouch, the LLVM 14 optimizer seems too aggressive. Derived from dmd-testsuite's runnable/nested.d:

import core.stdc.stdio;

void foo14846(Dg)(scope Dg code)
{
    code();
}

void test14846()
{
    int x;

    struct S
    {
        this(int n) { x = n; }
        ~this() { printf("dtor: %p, %p\n", &this, &x); x = 99; }
    }

    foo14846({ S* p = new S(1); });
}

void main()
{
    test14846();
}

This is what optimized main() looks like:

define i32 @_Dmain({ i64, { i64, i8* }* } %unnamed) #5 {
  %.gc_struct.i.i = tail call noalias i8* @_d_newitemT(%object.TypeInfo* bitcast (%object.TypeInfo_Struct* @_D28TypeInfo_S3mod9test14846FZ1S6__initZ to %object.TypeInfo*)) #1 ; [#uses = 0]
  ret i32 0
}

So it does the S GC allocation alone - without GC-allocating the nested context, initializing the hidden p.vthis field accordingly, modifying x in the S.this(int) constructor etc. Most likely because it has no idea that the instance is destructed indirectly via the GC at program termination...

Edit: With LLVM 13.0.1:

define i32 @_Dmain({ i64, { i64, i8* }* } %unnamed) #5 {
  %.frame.i = tail call noalias i8* @_d_allocmemory(i64 4) #1 ; [#uses = 2]
  %x.i = bitcast i8* %.frame.i to i32*            ; [#uses = 2]
  store i32 0, i32* %x.i, align 4
  %.gc_struct.i.i = tail call noalias i8* @_d_newitemT(%object.TypeInfo* bitcast (%object.TypeInfo_Struct* @_D28TypeInfo_S3mod9test14846FZ1S6__initZ to %object.TypeInfo*)) #1 ; [#uses = 1]
  %.vthis.i.i = bitcast i8* %.gc_struct.i.i to i8** ; [#uses = 1]
  store i8* %.frame.i, i8** %.vthis.i.i, align 8
  store i32 1, i32* %x.i, align 4
  ret i32 0
}

@JohanEngelen
Copy link
Member

Perhaps it is because of the noalias attribute that we put on the return value of new. Perhaps that means that noone else will meaningfully access the values put in that memory location, thus it is OK to remove stores to it, because the pointer does not escape our function --> data will never be used.
It's strange that it removes the _d_allocmemory completely, because that call could have side effects...

@kinke
Copy link
Member Author

kinke commented Apr 4, 2022

Right on, Johan - removing the noalias attribute from _d_newitemT seems to be enough to 'fix' the testcase.

From https://llvm.org/docs/LangRef.html#parameter-attributes:

On function return values, the noalias attribute indicates that the function acts like a system memory allocation function, returning a pointer to allocated storage disjoint from the storage for any other object accessible to the caller.

I think that should still apply here? I mean even malloc will need some book-keeping for the allocated chunks...

@kinke
Copy link
Member Author

kinke commented Apr 4, 2022

Filed llvm/llvm-project#54740.

…n prototypes

As that's invalid according to
llvm/llvm-project#54740, and a problem
since LLVM 14 at least.
@JohanEngelen
Copy link
Member

From https://llvm.org/docs/LangRef.html#parameter-attributes:

On function return values, the noalias attribute indicates that the function acts like a system memory allocation function, returning a pointer to allocated storage disjoint from the storage for any other object accessible to the caller.

I think that should still apply here? I mean even malloc will need some book-keeping for the allocated chunks...

I think the reasoning is that malloc needs to do bookkeeping, but it does not need to access the contents of memory allocated for the user. So whatever the users stores in it, it is not used by malloc. Our GC indirectly does access it in destructors.

@kinke
Copy link
Member Author

kinke commented Apr 4, 2022

Yeah that makes sense. 👍 (plus the GC might access it during scanning)

@kinke kinke marked this pull request as ready for review April 5, 2022 11:25
@kinke
Copy link
Member Author

kinke commented Apr 5, 2022

I propose keeping v13.0.1 for the prebuilt packages of the LDC v1.29 release, so that we don't skip LLVM 13, and to postpone v14 to LDC v1.30 (by simply reverting the last commit; CI is green with LDC-LLVM 14.0.0 as shown by the 2nd-latest commit).

@kinke kinke merged commit c5e4501 into ldc-developers:master Apr 6, 2022
@kinke kinke deleted the llvm14 branch April 6, 2022 14:15
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.

2 participants