Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Migrate GEPs to use opaque pointer compatable APIs #210

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

Merged
CodaFi merged 2 commits into llvm-swift:master from TristanBurnside:StructGEP2
Jun 5, 2019

Conversation

Copy link
Contributor

@TristanBurnside TristanBurnside commented Jun 2, 2019

Version 2 takes an explicit type parameter. This avoids a segmentation fault that happens when the pointer does not have an explicit type in LLVM

 

Version 2 takes an explicit type parameter. This avoids a segmentation fault that happens when the pointer does not have an explicit type in LLVM
Copy link
Member

CodaFi commented Jun 2, 2019 •
edited
Loading

(削除) Adding the type parameter defaulted is an API-compatible change. You can replace the brains of the original struct GEP inserter with this new binding and add the parameter. (削除ここまで)

///
/// - returns: A value representing the address of a subelement of the given
/// struct value.
public func buildStructGEP2(_ ptr: IRValue, type: IRType? = nil, index: Int, name: String = "") -> IRValue {
Copy link
Member

Choose a reason for hiding this comment

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

This API seems contra to the goals of the opaque pointer movement, right? We might have to go through a deprecation cycle after all...

Anyways, we shouldn’t try to infer the type from the pointer since that will eventually go away.

Copy link
Member

CodaFi commented Jun 2, 2019

Ignore my earlier comments. Matt was right (though his comment seems to have disappeared). We should expose this without the 2 and without the type parameter defaulted as an overload and deprecate the other overload. We’ll make this an error next release.

Copy link
Contributor Author

Ok, I'll change it to non default and no 2. I wasn't quite sure if they were functionally the same but I guess if the old one doesn't work with the latest LLVM it's a moot point.

Copy link
Member

CodaFi commented Jun 2, 2019

The 2 on LLVM’s side is because of a policy of never breaking ABI compatibility, which has the unfortunate side effect of basically never allowing us to remove and replace deprecated APIs. It’s a terrible frustration we have tried to avoid in our Swift bindings.

@TristanBurnside TristanBurnside changed the title (削除) Expose LLVMBuildStructGEP2 from the C API (削除ここまで) (追記) Migrate GEPs to use opaque pointer compatable APIs (追記ここまで) Jun 3, 2019
///
/// - returns: A value representing the result of a load from the given
/// pointer value.
public func buildLoad(type: IRType, ptr: IRValue, ordering: AtomicOrdering = .notAtomic, volatile: Bool = false, alignment: Alignment = .zero, name: String = "") -> IRInstruction {
Copy link
Member

Choose a reason for hiding this comment

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

In the interest of API flow, I think the pointer parameter should come before the type parameter. The leading parameter should retain its anonymity as well.

 public func buildLoad(_ ptr: IRValue, type: IRType, ordering: AtomicOrdering = .notAtomic, volatile: Bool = false, alignment: Alignment = .zero, name: String = "") -> IRInstruction

Copy link
Contributor Author

@TristanBurnside TristanBurnside Jun 4, 2019

Choose a reason for hiding this comment

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

I was mostly following the ordering from the underlying C API here, but also I read this as: build load, of type, from pointer, which (for me) flows better than the other way around.

I also don't think that the name here (buildLoad) implies that the first parameter must be a pointer, which I believe should be the requirement for anonymity.

Copy link
Contributor Author

@TristanBurnside TristanBurnside Jun 4, 2019

Choose a reason for hiding this comment

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

That said, these points are both pretty minor and purely stylistic, so I'll go ahead and change them.

/// Build a load instruction that loads a value from the location in the
/// given value.
///
/// - parameter type: The type of value to load.
Copy link
Member

Choose a reason for hiding this comment

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

I think we ought to be clearer that this is the type of the pointed-to value and not the type of the pointer value. Maybe

+ - parameter type: The type of the value loaded from the given pointer.

TristanBurnside reacted with thumbs up emoji
///
/// - returns: A value representing the address of a subelement of the given
/// aggregate data structure value.
public func buildInBoundsGEP(type: IRType, ptr: IRValue, indices: [IRValue], name: String = "") -> IRValue {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here.

public func buildGEP(type: IRType, ptr: IRValue, indices: [IRValue], name: String = "") -> IRValue {
var vals = indices.map { 0ドル.asLLVM() as Optional }
return vals.withUnsafeMutableBufferPointer { buf in
return LLVMBuildGEP2(llvm, type.asLLVM(), ptr.asLLVM(), buf.baseAddress, UInt32(buf.count), name)
Copy link
Member

Choose a reason for hiding this comment

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

And here.

/// - returns: A value representing the address of a subelement of the given
/// struct value.
public func buildStructGEP(type: IRType, ptr: IRValue, index: Int, name: String = "") -> IRValue {
return LLVMBuildStructGEP2(llvm, type.asLLVM(), ptr.asLLVM(), UInt32(index), name)
Copy link
Member

Choose a reason for hiding this comment

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

And here

Copy link
Member

CodaFi commented Jun 5, 2019

Thanks for your patience

⛵️

@CodaFi CodaFi merged commit 15f1656 into llvm-swift:master Jun 5, 2019
Copy link
Contributor Author

Thanks for all your help,

As a consumer, I am glad you keep high quality standards.

@TristanBurnside TristanBurnside deleted the StructGEP2 branch June 6, 2019 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodaFi CodaFi CodaFi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /