Repository navigation
Conversation
|
Thanks for opening a PR! This project follows LLVM's incremental-development and AI-tool-use Before requesting review, please check that:
Reviewers are assigned through |
L2 Accuracy Results (EP vs CPU)
Threshold: 0.01 | Run: 7880 - Commit: |
Binary sizesBaseline: run 40209447587 -
MorphiZen EP Performance Results
EPContext Export Performance
EPContext Import Performance
OGA Benchmark Results
OGA Wheel Smoke (Python benchmark_e2e.py)
VLM Benchmark Results
Run: 7880 - Commit: |
|
Hi @fhanuman, could you please help to review again? thanks! |
fhanuman
left a comment
There was a problem hiding this comment.
Could we also please make sure this change doesn’t cause any performance regression?
|
/run-test models=all |
|
ROCm GenAI CI finished with result: UNSTABLE.
|
2452fad to
9f3d537
Compare
|
Hi @fhanuman All review comments have been addressed. CI showed some performance variation for a few models, but the baseline and current results were collected on different machines. I also ran a controlled same-machine A/B test with only the pass order changed. The TPS differences were 0.76% and 0.35%, which are within normal run-to-run variation, so no measurable regression was observed. Could you please take another look? Thank you. |
| } | ||
| } | ||
|
|
||
| builder.setInsertionPoint(allocOp); |
There was a problem hiding this comment.
What is the expectation with this new change for the below IR? Won't this be a problem?
func.func @main_graph(
%ctx: !hip.context, %a: index, %b: index)
-> memref<4x4xf32> {
// Assume %a = 4 and %b = 4 at runtime.
%buffer = memref.alloc() : memref<16xf32>
%dynamic = memref.cast %buffer
: memref<16xf32> to memref<?xf32>
%expanded = memref.expand_shape %dynamic [[0, 1]]
output_shape [%a, %b]
: memref<?xf32> into memref<?x?xf32>
%result = memref.cast %expanded
: memref<?x?xf32> to memref<4x4xf32>
return %result : memref<4x4xf32>
}
| /// corresponding input dimension (ONNX zero with allowzero=0), equal its | ||
| /// static extent, or come directly from tensor.dim(data, i). Unknown entries, | ||
| /// including -1, conservatively keep the Reshape. | ||
| static bool isProvablyIdentityReshape(mlir::Operation *op, mlir::Value data, |
There was a problem hiding this comment.
I see around a 20% drop in performance in some cases in the comparison report
(http://xsjnts.xilinx.com/ROCm/compare?base_url=http%3A%2F%2Fxsjnts.xilinx.com%2FROCm%2Fhipdnn-detail%3Fids%3D6ad9baedf27f47880d1b66a8%26collection%3DHIP-EP-Dynamic&base_job=&base=&curr_url=&curr_job=daily_hip_ep_genai&curr=2584),
so I am not sure we can attribute this to machine differences. Please confirm this once on the same machine, with and without all the changes in this PR for all the models that we run in the CI/CD.
For example, please see the PR below for how I have been qualifying the changes:
We should make sure our compiler changes do not cause performance regressions. @amd-mingw, @qianglin-amd , could you please share your thoughts here?
9f3d537 to
f2d54d3
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
f2d54d3 to
0317216
Compare
Summary
Improve correctness of dynamic-shape lowering, output allocation, device copies, and temporary-buffer handling.
The changes preserve runtime dimensions through Reshape and Slice lowering, keep eligible copies on device paths, allocate graph outputs with the correct external shape, and materialize host-written scalar buffers before generic allocation reuse.
Related issue or design
None.
Why
Several transformations relied on information that is insufficient for dynamic shapes:
These cases could produce incorrect output extents, stale shape values, invalid host-side copies of device memory, or output descriptors with the wrong rank.
What
OptimizeMemRefstarget-independent and run layout promotion after reuse.Test plan
Performance
The pass reorder does not introduce additional kernels, copies, or synchronization.
The Slice fallback uses the shared scalar-readback infrastructure. In the validated graph, the required scalar was already host-visible through an earlier shape operation, so the readback count did not increase and no measurable performance regression was observed.
Ordinary device allocations continue to use
OptimizeMemRefs; only recognized host-written scalar staging allocations are moved to host scratch before reuse.Notes for reviewers
MaterializeHostScalarshandles target-specific host placement before the generic reuse pass.PromoteStridedHipOperandsremains afterOptimizeMemRefsso layouts introduced during reuse are normalized before pooling.AI assistance disclosure
AI assistance was used to analyze compiler IR behavior, review the implementation, draft regression tests, investigate failures, and prepare the pull request description. All changes were reviewed, understood, built, and validated with pre-commit, targeted tests, and end-to-end smoke tests.
Checklist