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

Support typed LLVM getelementptr instructions #12623

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Oct 17, 2022

This implements part of #12484 by allowing LLVM::Builder#gep and #inbounds_gep to accept an optional type argument. Given an LLVM instruction like:

%1 = getelementptr %T, %U* %0, i64 7

To maintain the same semantics before this change, %T and %U should remain identical types from the same context. There are now four possible scenarios:

  • type is given, LLVM ≥ 8.0: %T is type, %U comes from value;
  • type is omitted, LLVM ≥ 8.0: %T is value.type, %U comes from value;
  • type is given, LLVM < 8.0: %T and %U come from value. The type argument is silently ignored;
  • type is omitted, LLVM < 8.0: %T and %U come from value.

The compiler itself provides the type argument in all cases. The builder now always uses LLVMBuildGEP2 / LLVMBuildInBoundsGEP2 instead of the deprecated methods if LLVM 8.0 or above is detected. There are no breaking changes in this patch, and all the codegen specs have been locally tested on Nix shells for LLVM 7 and 14 on aarch64-apple-darwin.

Later a similar migration has to be done for #load / LLVMBuildLoad too.

@HertzDevil
Copy link
Contributor Author

I have no idea what went wrong with the CI since I do not have an x86_64-darwin machine and LLVM 11.1 works locally.

@beta-ziliani beta-ziliani mentioned this pull request Jan 6, 2023
6 tasks
@beta-ziliani
Copy link
Member

I got the following error in LLVM 11.1.0 compiled with assertions:

Assertion failed: (PointeeType == cast<PointerType>(Ptr->getType()->getScalarType())->getElementType()), function Create, file Instructions.h, line 941.

This is the assert failing: https://github.com/llvm/llvm-project/blob/1fdec59bffc11ae37eb51a1b9869f0696bfd5312/llvm/include/llvm/IR/Instructions.h#L939

@HertzDevil HertzDevil changed the base branch from master to feature/llvm-opaque-pointers January 14, 2023 11:49
@HertzDevil
Copy link
Contributor Author

I have created a separate feature branch for the opaque pointer migrations. Let's keep them there until the whole set is complete

@HertzDevil HertzDevil merged commit c20d8cc into crystal-lang:feature/llvm-opaque-pointers Jan 16, 2023
@HertzDevil HertzDevil deleted the feature/llvm-typed-gep branch January 18, 2023 13:21
@@ -731,7 +731,7 @@ class Crystal::CodeGenVisitor
allocate_aggregate base_type

unless type.struct?
type_id_ptr = aggregate_index(@last, 0)
type_id_ptr = aggregate_index(llvm_struct_type(type), @last, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm quite certain that here is the problem. I looked into it because "unless struct get struct type" sounded weird. Locally I can build fine with every other place using gep2 but here. This said, I couldn't fix it and, weirdly, removing entirely this code (unless ... end) makes everything work. @HertzDevil if you have any clue, let me know, otherwise we call Ary.

Copy link
Contributor Author

@HertzDevil HertzDevil Mar 3, 2023

Choose a reason for hiding this comment

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

The llvm_type of any reference type is a pointer, llvm_struct_type is the pointee's layout:

class Foo
  @x = ""
end

# type                   # => Foo
# llvm_struct_type(type) # => %Foo = type { i32, %String* }
# llvm_type(type)        # => %Foo*
Foo.new

And indeed, @last's LLVM type is llvm_struct_type(type) due to allocate_aggregate, regardless of whether Foo is a value or reference.

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

Successfully merging this pull request may close these issues.

3 participants