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.

Sign up for GitHub

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

Integrate llvm-project @4f3c9dabecc6074f8455ca23ba70020d5c556e63 #17827

Merged
merged 6 commits into from
Jul 9, 2024
Merged

Integrate llvm-project @4f3c9dabecc6074f8455ca23ba70020d5c556e63 #17827

merged 6 commits into from
Jul 9, 2024

Conversation

yzhang93
Copy link
Contributor

@yzhang93 yzhang93 commented Jul 8, 2024 •
edited by hanhanW
Loading

@google-cla Google CLA
Copy link

google-cla bot commented Jul 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@yzhang93
Start LLVM integrate integrate-llvm-20240708
60d1774 
Signed-off-by: yzhang93 <zhyuhang88@gmail.com>
@yzhang93
Advance LLVM to a582dde192d1b9596f3a2609275f04fd450e4aec: [mlir][c-ap…
eab50ac 
…i] expose elideLargeResourceString (#98050) (Brendan Hansknecht on 2024-07-08 14:12:49 -0700) (29 of 31)

Signed-off-by: yzhang93 <zhyuhang88@gmail.com>
@yzhang93
Fix to use OpOperand* in ControlPropagationFn
a99394c 
Signed-off-by: yzhang93 <zhyuhang88@gmail.com>
@hanhanW
fixes for upstream data-layout propagation changes.
f0573fb 
Signed-off-by: hanhanW <hanhan0912@gmail.com>
hanhanW
hanhanW reviewed Jul 9, 2024
View reviewed changes
compiler/src/iree/compiler/Codegen/Dialect/GPU/Transforms/PackToIntrinsics.cpp
linalg::ControlPropagationFn control = [](OpOperand *opOperand) -> bool {
Operation *producer = opOperand->get().getDefiningOp();
Operation *consumer = opOperand->getOwner();
return !getLoweringConfig(producer) || !getLoweringConfig(consumer);
Copy link
Contributor

Choose a reason for hiding this comment

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

@qedawkins is the change correct? Do we check both operations, or do we only check producer or consumer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want && because one of the operations will be a pack/unpack and the other will be the to-be-packed op.

@yzhang93 yzhang93 enabled auto-merge (squash) July 9, 2024 20:26
qedawkins
qedawkins approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

LGTM with the one amendment Hanhan asked about

compiler/src/iree/compiler/Codegen/Dialect/GPU/Transforms/PackToIntrinsics.cpp
linalg::ControlPropagationFn control = [](OpOperand *opOperand) -> bool {
Operation *producer = opOperand->get().getDefiningOp();
Operation *consumer = opOperand->getOwner();
return !getLoweringConfig(producer) || !getLoweringConfig(consumer);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want && because one of the operations will be a pack/unpack and the other will be the to-be-packed op.

hanhanW
hanhanW reviewed Jul 9, 2024
View reviewed changes
Copy link
Contributor

@hanhanW hanhanW left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I can send a follow-up if Quinn has some comments. The only concern is about the benchmark result.

@qedawkins
Copy link
Contributor

The changes look good to me. I can send a follow-up if Quinn has some comments. The only concern is about the benchmark result.

Sounds good, no need to block on my comment because I see that CI is being finicky and it's a path we don't hit anywhere yet.

@hanhanW hanhanW changed the title Integrate llvm-project 20240708 Integrate llvm-project @4f3c9dabecc6074f8455ca23ba70020d5c556e63 Jul 9, 2024
@hanhanW hanhanW disabled auto-merge July 9, 2024 20:33
@hanhanW hanhanW enabled auto-merge (squash) July 9, 2024 20:34
@hanhanW hanhanW merged commit 8b8df59 into main Jul 9, 2024
59 of 60 checks passed
@hanhanW hanhanW deleted the integrate-llvm-20240708 branch July 9, 2024 21:05
hanhanW added a commit that referenced this pull request Jul 11, 2024
@hanhanW
[GPU] Fix the propagation control function logic.
729744b 
This is a follow-up from the integrate comment: #17827 (comment)

The comment was not addressed in the integrate PR because the CI was not stable. Some runners were off, and we decided to land the PR and sent a follow-up later. See https://discord.com/channels/689900678990135345/1080178290188374049/1260330876651307090 for some details.
@hanhanW hanhanW mentioned this pull request Jul 11, 2024
Merged
hanhanW added a commit that referenced this pull request Jul 11, 2024
@hanhanW
[GPU] Fix the propagation control function logic. (#17869)
20d8308 
This is a follow-up from the integrate comment:
#17827 (comment)

The comment was not addressed in the integrate PR because the CI was not
stable. Some runners were off, and we decided to land the PR and sent a
follow-up later. See
https://discord.com/channels/689900678990135345/1080178290188374049/1260330876651307090
for more details.
saienduri pushed a commit to saienduri/iree that referenced this pull request Jul 12, 2024
@hanhanW @saienduri
[GPU] Fix the propagation control function logic. (iree-org#17869)
0b75bd4 
This is a follow-up from the integrate comment:
iree-org#17827 (comment)

The comment was not addressed in the integrate PR because the CI was not
stable. Some runners were off, and we decided to land the PR and sent a
follow-up later. See
https://discord.com/channels/689900678990135345/1080178290188374049/1260330876651307090
for more details.
saienduri pushed a commit to saienduri/iree that referenced this pull request Jul 12, 2024
@hanhanW @saienduri
[GPU] Fix the propagation control function logic. (iree-org#17869)
9157071 
This is a follow-up from the integrate comment:
iree-org#17827 (comment)

The comment was not addressed in the integrate PR because the CI was not
stable. Some runners were off, and we decided to land the PR and sent a
follow-up later. See
https://discord.com/channels/689900678990135345/1080178290188374049/1260330876651307090
for more details.

Signed-off-by: saienduri <saimanas.enduri@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanhanW hanhanW hanhanW approved these changes

@qedawkins qedawkins qedawkins approved these changes

@MaheshRavishankar MaheshRavishankar Awaiting requested review from MaheshRavishankar MaheshRavishankar is a code owner

@antiagainst antiagainst Awaiting requested review from antiagainst antiagainst is a code owner

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@yzhang93 @qedawkins @hanhanW @kuhar