-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Migrate SwiftCompilerSources to FunctionConvention #70535
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
Conversation
cd29718
to
bb2b64f
Compare
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.
Nice! Much better than before!
Just a few comments about naming.
I'm almost tempted to introduce struct ApplyArgumentIndex
and struct CalleeArgumentIndex
to let the type checker rule out wrong uses of the various index kinds.
return calleeArgumentConventions[argIdx] | ||
} | ||
|
||
public func calleeArgumentIndex(of index: Int) -> Int? { |
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.
better: calleeArgumentIndex(ofOperandIndex: Int)
to make it clear that the index is not an argument but the operand index
operandConventions.convention(of: operand) | ||
} | ||
|
||
public func calleeArgumentOperand(at calleeArgIdx: Int) -> Operand? { |
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 name is wrong. callee
-> caller
Also we should specify the kind of index in the argument label: callerArgumentOperand(forCalleeArgumentIndex:)
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.
public func operand(forCalleeArgumentIndex calleeArgIdx: Int) -> Operand?
/// ``` | ||
func calleeArgIndex(callerArgIndex: Int) -> Int | ||
|
||
/// Converts an argument index of a callee to the corresponding argument index of the apply. |
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 you move this comment to callerArgumentOperand(forCalleeArgumentIndex:)
?
return index + 1 | ||
} | ||
|
||
public subscript(_ index: Int) -> ArgumentConvention { |
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 you rename index
-> operandIndex
, so that it's clear that it is not the argument index?
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.
Done. There is still no subscript label, because it can only by used to subscript a collection of operand conventions:
operandConventions[operand.index]
Index wrapper types would also be good. I didn't want to go quite that far yet. But this PR goes a long way toward eliminating ambiguity two ways
|
ce4bf7c
to
f5a8001
Compare
@swift-ci test |
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.
lgtm!
operandConventions.convention(of: operand) | ||
} | ||
|
||
// Converts an argument index of a callee to the corresponding apply operand. |
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 you move the comment of the original callerArgIndex(calleeArgIndex: Int)
here (with s/caller indices/caller operands/
)? Something like
/// If the apply does not actually apply that argument, it returns nil.
///
/// Example:
/// ```
/// func callee(v, w, x, y, z) { }
/// // caller operands in %pa: -, -, c, d, e ("-" == nil)
/// // caller operands in %a: a, b, -, -, -
///
/// %pa = partial_apply @callee(c, d, e)
/// %a = apply %pa (a, b)
/// ```
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, that's great!
f5a8001
to
8905fc9
Compare
@swift-ci test |
@swift-ci smoke test |
Layers: - FunctionConvention: AST FunctionType: results, parameters - ArgumentConventions: SIL function arguments - ApplyOperandConventions: applied operands The meaning of an integer index is determined by the collection type. All the mapping between the various indices (results, parameters, SIL argument, applied arguments) is restricted to the collection type that owns that mapping. Remove the concept of a "caller argument index".
8905fc9
to
2128c21
Compare
@swift-ci smoke test |
Layers:
- FunctionConvention: AST FunctionType: results, parameters
- ArgumentConventions: SIL function arguments
- ApplyOperandConventions: applied operands
The meaning of an integer index is determined by the collection
type. All the mapping between the various indices (results,
parameters, SIL argument, applied arguments) is restricted to the
collection type that owns that mapping. Remove the concept of a
"caller argument index".