-
Notifications
You must be signed in to change notification settings - Fork 200
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
NativeAOT-LLVM: Upgrade to LLVM v15 and opaque pointers #2139
Conversation
upgrade llvm use LLVM opaque pointers in the clrjit module only. add nuget ref for LLVMSharp v15
cc @dotnet/nativeaot-llvm |
I added a |
- Configure the LLVM source to use the same runtime as the Jit: `cmake -G "Visual Studio 17 2022" -DCMAKE_BUILD_TYPE=Debug -D LLVM_USE_CRT_DEBUG=MTd path-to-the-build-directory` or if building for the Release configuration `cmake -G "Visual Studio 17 2022" -DCMAKE_BUILD_TYPE=Release -D LLVM_USE_CRT_RELEASE=MT path-to-the-build-directory` | ||
- Download the LLVM 15.0.6 source from https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.6/lldb-15.0.6.src.tar.xz | ||
- Extract it and create a subdirectory in the `llvm-15.0.6.src` folder (`path-to-the-build-directory`). Do not create at the root of a drive. | ||
- Download the CMake 15.0.6 source from https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.6/cmake-15.0.6.src.tar.xz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, custom CMake is quite an unusual requirement. Why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hit this problem: llvm/llvm-project#57573 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad. I suppose there is nothing we can do then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I've not fixed then yml scripts... better do that.
src/coreclr/jit/llvmcodegen.cpp
Outdated
@@ -552,10 +552,10 @@ Value* Llvm::consumeValue(GenTree* node, Type* targetLlvmType) | |||
return _builder.CreatePtrToInt(nodeValue, Type::getInt32Ty(_llvmContext)); | |||
} | |||
|
|||
// i32* e.g symbols, to i8* | |||
// all pointers are opaque | |||
if (nodeValue->getType()->isPointerTy() && targetLlvmType->isPointerTy()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be deleted then? Since all pointer types are (at the moment at least, since we don't use address spaces) are, presumably, equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it can, thanks
src/coreclr/jit/llvmcodegen.cpp
Outdated
if (lclAddr->OperIs(GT_LCL_FLD_ADDR)) | ||
{ | ||
Value* bytePtr = castIfNecessary(getLocalAddr(lclNum), Type::getInt8PtrTy(_llvmContext)); | ||
mapGenTreeToValue(lclAddr, gepOrAddr(bytePtr, lclAddr->GetLclOffs())); | ||
mapGenTreeToValue(lclAddr, gepOrAddr(localAddr, lclAddr->GetLclOffs())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can delete the branch (if (lclAddr->OperIs
...) now, since it will work for both LCL_VAR_ADDR
and LCL_FLD_ADDR
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks like it.
with the update to emsdk, we need a warning surpressed.
Still got zlib problems, will try cherry picking dotnet/runtime#73082 |
- zlib updated to v1.2.12 - zlib-intel updated to v1.2.12_jtk.1 Also required additional minor tweaks (documented in zlib-version.txt and zlib-intel-version.txt) to get the build to work.
I'm going to leave this as I can't find a way to build zlib with the latest emscripten, hopefully a merge from runtime will fix it in the future. |
Changing the correct cmakelist.txt helped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is a great change! Opaque pointers will make things faster and simpler for us.
src/coreclr/jit/llvmcodegen.cpp
Outdated
@@ -1254,7 +1239,8 @@ void Llvm::buildCnsInt(GenTree* node) | |||
{ | |||
const char* symbolName = GetMangledSymbolName((void*)(node->AsIntCon()->IconValue())); | |||
AddCodeReloc((void*)node->AsIntCon()->IconValue()); | |||
mapGenTreeToValue(node, _builder.CreateLoad(getOrCreateExternalSymbol(symbolName))); | |||
mapGenTreeToValue(node, _builder.CreateLoad(Type::getInt8PtrTy(_llvmContext), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getInt8PtrTy
Shouldn't we be always using PointerType::getUnqual
from now on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks, fixed.
@@ -1722,13 +1706,13 @@ void Llvm::storeObjAtAddress(Value* baseAddress, Value* data, StructDesc* struct | |||
{ | |||
// We can't be sure the address is on the heap, it could be the result of pointer arithmetic on a local var. | |||
emitHelperCall(CORINFO_HELP_CHECKED_ASSIGN_REF, | |||
{address, castIfNecessary(fieldData, Type::getInt8PtrTy(_llvmContext))}); | |||
{address, castIfNecessary(fieldData, llvm::PointerType::getUnqual(_llvmContext))}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, when is fieldData
not a pointer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. Its the RhpCheckedAssignRef
for S_P_TypeLoader_Internal_TypeSystem_Instantiation___cctor
:
Generating: [000043] ------------ t43 = LCL_VAR int V01 tmp1 u:1 (last use)
/--* t43 int
Generating: N005 ( 3, 2) [000007] n----------- t7 = * OBJ struct<Internal.TypeSystem.Instantiation, 4>
%16 = load i32, ptr %0, align 4, !dbg !5
/--* t10 byref
+--* t7 struct
Generating: N004 ( 22, 8) [000012] -ACXG------- * STORE_OBJ struct<Internal.TypeSystem.Instantiation, 4> (copy)
Instantiation
is
public struct Instantiation : IEquatable<Instantiation>
{
private TypeDesc[] _genericParameters;
So is this because of the LLVM struct representation of an i32
I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Bit unexpected (one would think the struct would be normalized to ptr
), but also pre-existing so nothing to do.
src/coreclr/jit/llvmcodegen.cpp
Outdated
Value* srcAddress = | ||
_builder.CreateGEP(Type::getInt8Ty(_llvmContext), baseAddress, _builder.getInt32(bytesStored)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this use gepOrAddr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, changed
// S_P_CoreLib_System_Globalization_UmAlQuraCalendar___cctor | ||
var mangledName = NodeFactory.NameMangler.GetMangledMethodName(method).ToString(); | ||
// if (mangledName == "S_P_CoreLib_System_Globalization_UmAlQuraCalendar___cctor") | ||
// { | ||
var sig = method.Signature; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// S_P_CoreLib_System_Globalization_UmAlQuraCalendar___cctor | |
var mangledName = NodeFactory.NameMangler.GetMangledMethodName(method).ToString(); | |
// if (mangledName == "S_P_CoreLib_System_Globalization_UmAlQuraCalendar___cctor") | |
// { | |
var sig = method.Signature; | |
var mangledName = NodeFactory.NameMangler.GetMangledMethodName(method).ToString(); | |
var sig = method.Signature; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, removed that debug stuff
// } | ||
// else ILImporter.CompileMethod(this, methodCodeNodeNeedingCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// } | |
// else ILImporter.CompileMethod(this, methodCodeNodeNeedingCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -60,6 +65,8 @@ public sealed class LLVMCodegenCompilation : RyuJitCompilation | |||
DIBuilder = Module.CreateDIBuilder(); | |||
DebugMetadataMap = new Dictionary<string, DebugMetadata>(); | |||
ILImporter.Context = Module.Context; | |||
LLVMContextSetOpaquePointers(Module.Context, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious how this works linking wise. Does the Jit module still produce declarations/definitions of functions using precise pointer types, or...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Jit module
declare i8 @S_P_CoreLib_System_Runtime_CompilerServices_RuntimeHelpers__Equals(ptr)
In the IL module
define i8 @S_P_CoreLib_System_Runtime_CompilerServices_RuntimeHelpers__Equals(i8* %0)
This is used in Object__Equals
from the Jit module. You can run wasm2wat on the final wasm and you see:
(func $Object__Equals (type 1) (param i32) (result i32)
(local i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32)
local.get 0
i32.load
local.set 1
i32.const 4
local.set 2
local.get 0
local.get 2
i32.add
local.set 3
local.get 3
i32.load
local.set 4
i32.const 8
local.set 5
local.get 0
local.get 5
i32.add
local.set 6
i32.const 8
local.set 7
local.get 0
local.get 7
i32.add
local.set 8
local.get 8
local.get 1
i32.store
i32.const 12
local.set 9
local.get 0
local.get 9
i32.add
local.set 10
local.get 10
local.get 4
i32.store
local.get 6
call $S_P_CoreLib_System_Runtime_CompilerServices_RuntimeHelpers__Equals
As Wasm just uses i32 for the pointers, I guess the linker doesn't really care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Presumably means we can drop all of GetParameterType
/GetArgTypeIncludingParameterized
(in a follow up change) as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wat for $S_P_CoreLib_System_Runtime_CompilerServices_RuntimeHelpers__Equals
func $S_P_CoreLib_System_Runtime_CompilerServices_RuntimeHelpers__Equals (type 1) (param i32) (result i32)
Where type 1
is
(type (;1;) (func (param i32) (result i32)))
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
remove debug code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our "normal"
Failures, retrying |
hurrah! |
Thanks for the packages and the reviews! |
This PR upgrades emsdk, and LLVM. LLVM is moving to opaque pointers where the pointers have no types. E.g.
i8*
becomesptr
. LLVMload
s need the type specified. All thebitcast
noops are gone from the clrjit module. I didn't do the the IL module as it's going.Note, there are some changes to the setup instructions for LLVM as it now requires the cmake tarball downloaded and installed.
Closes: #2138