-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[AMDGPU] Unused sdst writing to null #133229
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -973,11 +973,13 @@ bool SIShrinkInstructions::run(MachineFunction &MF) { | |
| continue; | ||
| } | ||
|
|
||
| if (!TII->hasVALU32BitEncoding(MI.getOpcode())) { | ||
| // If there is no chance we will shrink it and use VCC as sdst to get | ||
| // a 32 bit form try to replace dead sdst with NULL. | ||
| // If there is no chance we will shrink it and use VCC as sdst to get | ||
| // a 32 bit form try to replace dead sdst with NULL. | ||
| if (TII->isVOP3(MI.getOpcode())) { | ||
| tryReplaceDeadSDST(MI); | ||
| continue; | ||
| if (!TII->hasVALU32BitEncoding(MI.getOpcode())) { | ||
| continue; | ||
| } | ||
|
Comment on lines
+980
to
+982
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't see the point of adding this check here. What does it buy us? If you want to save some execution time it would be better for Also nit: don't need the braces.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skipping this check will lead to a change in a lot of tests and illegal virtual register errors in some of them |
||
| } | ||
|
|
||
| if (!TII->canShrink(MI, *MRI)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,8 @@ define amdgpu_ps void @v_add_u64(ptr addrspace(1) %out, i64 %a, i64 %b) { | |
| ; GCN-LABEL: v_add_u64: | ||
| ; GCN: ; %bb.0: ; %entry | ||
| ; GCN-NEXT: v_add_co_u32 v2, vcc_lo, v2, v4 | ||
| ; GCN-NEXT: v_add_co_ci_u32_e32 v3, vcc_lo, v3, v5, vcc_lo | ||
| ; GCN-NEXT: s_delay_alu instid0(VALU_DEP_1) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adds an extra instruction, is this really better?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delay alu pass skips implicit def ops (suppose that should be done since it's marked with TODO). VOP2: VOP3: In VOP2, vcc is implicit so delay alu will not consider it, but as I understood the correct behaviour would be for delay_alu to be in both?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two things going on here:
|
||
| ; GCN-NEXT: v_add_co_ci_u32_e64 v3, null, v3, v5, vcc_lo | ||
| ; GCN-NEXT: global_store_b64 v[0:1], v[2:3], off | ||
| ; GCN-NEXT: s_endpgm | ||
| entry: | ||
|
|
@@ -85,7 +86,8 @@ define amdgpu_ps void @v_sub_u64(ptr addrspace(1) %out, i64 %a, i64 %b) { | |
| ; GCN-LABEL: v_sub_u64: | ||
| ; GCN: ; %bb.0: ; %entry | ||
| ; GCN-NEXT: v_sub_co_u32 v2, vcc_lo, v2, v4 | ||
| ; GCN-NEXT: v_sub_co_ci_u32_e32 v3, vcc_lo, v3, v5, vcc_lo | ||
| ; GCN-NEXT: s_delay_alu instid0(VALU_DEP_1) | ||
| ; GCN-NEXT: v_sub_co_ci_u32_e64 v3, null, v3, v5, vcc_lo | ||
| ; GCN-NEXT: global_store_b64 v[0:1], v[2:3], off | ||
| ; GCN-NEXT: s_endpgm | ||
| entry: | ||
|
|
||
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.
Comment needs updating. We are now trying to replace dead sdst with null even if there is a chance that we could shrink to a 32-bit encoding.