Migrate GEPs to use opaque pointer compatable APIs #210
Conversation
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
|
|
Sources/LLVM/IRBuilder.swift
Outdated
| /// | ||
| /// - 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 { |
There was a problem hiding this comment.
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.
|
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. |
|
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. |
|
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
changed the title
Sources/LLVM/IRBuilder.swift
Outdated
| /// | ||
| /// - 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 { |
There was a problem hiding this comment.
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 = "") -> IRInstructionThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
That said, these points are both pretty minor and purely stylistic, so I'll go ahead and change them.
Sources/LLVM/IRBuilder.swift
Outdated
| /// Build a load instruction that loads a value from the location in the | ||
| /// given value. | ||
| /// | ||
| /// - parameter type: The type of value to load. |
There was a problem hiding this comment.
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.
Sources/LLVM/IRBuilder.swift
Outdated
| /// | ||
| /// - 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 { |
| 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) |
| /// - 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) |
Marks previous versions as deprecated
TristanBurnside
force-pushed
the
StructGEP2
branch
from
1157afe to
7802214
Compare
|
Thanks for your patience ⛵️ |
|
Thanks for all your help, As a consumer, I am glad you keep high quality standards. |
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