Improve the remove layout in handling loop op. - #7634
Conversation
c471fca to
4d0d68a
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the Intel GPU RemoveLayoutConversions pass to better handle loop-related values during backward/forward rematerialization, aiming to avoid duplicated loop bodies and improve kernel performance (referenced in issue #7604).
Changes:
- Extend forward propagation to traverse/control loop ops and handle
tt.descriptor_storesource rewrites. - Improve forward-propagation candidate collection to account for loop iter-args / loop results (to prevent duplicated loop bodies after rematerialization).
- Add an MLIR regression test reproducing the loop-related inefficiency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| third_party/intel/lib/TritonIntelGPUTransforms/RemoveLayoutConversions.cpp | Updates rematerialization forward-propagation logic and loop-related candidate selection; adds extra debug logging. |
| test/TritonIntelGPU/RemoveLayoutConversions/remove_layout_conversions_7625.mlir | Adds a regression test covering loop handling in remove-layout-conversions. |
4d0d68a to
d046f06
Compare
e9c8adc to
1c3b5f1
Compare
| @@ -1478,10 +1481,6 @@ | |||
There was a problem hiding this comment.
The removal of the scf::ForOp/scf::IfOp skip is too broad. The needRewrite path below calls rewriteOp(&op, ...) which is not designed for structured control flow ops — if a ForOp result happens to be in valuesToPropagate, this will attempt to rewrite the loop itself.
Consider keeping the skip for the needRewrite path while still entering regions for the DescriptorStoreOp case.
There was a problem hiding this comment.
Add back the guard of the rewriteOp. The value in the valuesToPropagate is chosen conservitively of the element-wise operations. But anyway we have added the protection.
1c3b5f1 to
257c021
Compare
Signed-off-by: Lu,Chengjun <chengjun.lu@intel.com>
Co-authored-by: Ettore Tiotto <ettore.tiotto@intel.com>
Signed-off-by: Lu,Chengjun <chengjun.lu@intel.com>
Signed-off-by: Lu,Chengjun <chengjun.lu@intel.com>
etiotto
left a comment
There was a problem hiding this comment.
LGTM.
One optional non-blocker: the assert(newOperand) in the DescriptorStoreOp path (RemoveLayoutConversions.cpp) compiles out in release builds — an if (!newOperand) continue; would be safer. (The dead sliceOps is noted inline.) Neither blocks merge.
Signed-off-by: Lu,Chengjun <chengjun.lu@intel.com>
This PR improves the Intel GPU RemoveLayoutConversions pass to better handle loop-related values during backward/forward rematerialization, aiming to avoid duplicated loop bodies and improve kernel performance (referenced in issue #7604).
chunk_gated_delta_rule_fwd_kernel_h_blockdim64 kernel time 207us vs 235us in #7604