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

Jump to bottom

[SR-13903] Make ApplyInst a MultipleValueInstruction #56301

Open
dan-zheng opened this issue Nov 27, 2020 · 10 comments
Open

[SR-13903] Make ApplyInst a MultipleValueInstruction #56301

dan-zheng opened this issue Nov 27, 2020 · 10 comments
Labels
compiler The Swift compiler in itself good first issue Good for newcomers improvement SILOptimizer Area → compiler: SIL optimization passes

Comments

@dan-zheng
Copy link
Collaborator

Previous ID SR-13903
Radar rdar://problem/71913175
Original Reporter @dan-zheng
Type Improvement
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Improvement, SILOptimizer, StarterBug
Assignee sachinvas16 (JIRA)
Priority Medium

md5: 41f0f57e0ecea221993e51084b3f1975

Issue Description:

Forum question with context: https://forums.swift.org/t/make-applyinst-a-multiplevalueinstruction/42294

SIL supports instructions that return multiple values (i.e. multiple result {{SILValue}}s).

However, apply instructions don't return multiple values. Instead, they produce a single value, which may have a tuple type. This leads to much avoidable destructuring and restructuring of tuples, which complicates SIL transformations like autodiff:

// example.swift
@_silgen_name("foo")
func foo() -> (Int, Int) {
  (0, 0)
}

@_silgen_name("bar")
func bar() -> (Int, Int) {
  return foo()
}
$ swiftc -emit-silgen example.swift
sil hidden [ossa] @bar : $@convention(thin) () -> (Int, Int) {
bb0:
  %0 = function_ref @foo : $@convention(thin) () -> (Int, Int)
  %1 = apply %0() : $@convention(thin) () -> (Int, Int)
  (%2, %3) = destructure_tuple %1 : $(Int, Int) // this is not good
  %4 = tuple (%2 : $Int, %3 : $Int)
  return %4 : $(Int, Int)
}

Instead, we can change ApplyInst to inherit MultipleResultInstruction.

Steps:

1. Change definition: make ApplyInst inherit MultipleValueInstruction.

Fix obvious compilation errors in the compiler codebase. Fix as many tests as possible.

2. Update and fix ApplyInst users: SIL generation + parsing + verification + printing, analyses and transformations, IRGen.
@typesanitizer
Copy link

@swift-ci create

@swift-ci
Copy link
Collaborator

Comment by Sachin Vas (JIRA)

theindigamer (JIRA User) Anyone working on this? Should I pick it up?

@typesanitizer
Copy link

I don't think anyone is, feel free to assign it to yourself (see Assignee field in top right of web UI) and work on it. 🙂

@typesanitizer
Copy link

I think this change will need to be broken up into at least 2 PRs. See https://forums.swift.org/t/make-applyinst-a-multiplevalueinstruction/42294/3 for context.

@swift-ci
Copy link
Collaborator

Comment by Saidhon Orifov (JIRA)

@dan-zheng theindigamer (JIRA User) based on my understanding from the forum, this has 2 parts to it, which will need to be broken down into 2 PR's:

  1. Change ApplyInst inheritance from SingleValueInstruction -> MultipleValueInstruction

    1. Basically after this change, I am seeing a couple of errors, and most likely tests failing, so will need to fix that, which makes sense.

  2. Update SIL generation + parsing + verification + printing, analyses and transformations, IRGen (which I didn't really quite follow tbh, is there more context on this or any guidance?)

@swift-ci
Copy link
Collaborator

Comment by Sachin Vas (JIRA)

saidhon (JIRA User) Are you working on this issue?

@dan-zheng
Copy link
Collaborator Author

sachinvas16 (JIRA User): I'll go ahead and assign you to this issue, if you'd like to get started!

As Varun said, you have the ability to assign yourself. Please do so in the future.

Please also change the issue status to "In Progress" after you do get started. This communicates to everyone that active progress is being made by you and that we can expect to see some PR within a few days or weeks. Certainly do not hesitate to ask questions (even small ones) if you get stuck, there are many experts here to help.

Hope this makes sense, thanks!

@swift-ci
Copy link
Collaborator

Comment by Sachin Vas (JIRA)

Thank you @dan-zheng, will get started ASAP.

@dan-zheng
Copy link
Collaborator Author

Thanks for the quick reply! I'm excited to see your progress 🙂

@swift-ci
Copy link
Collaborator

Comment by Saidhon Orifov (JIRA)

sachinvas16 (JIRA User) Yeah, but I yield back, feel free to take over. Sorry, I got started on it but never assigned it to myself or saw your comments.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler The Swift compiler in itself good first issue Good for newcomers improvement SILOptimizer Area → compiler: SIL optimization passes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dan-zheng @typesanitizer @swift-ci