[Codegen] Fix use after erase bug in optimize-tensor-extract-slices #19460
Conversation
| continue; | ||
| } | ||
| bool changed = true; | ||
| while (changed) { |
There was a problem hiding this comment.
Will review more when not on a phone, but this feels like a usecase for do { ... } while(changed)
compiler/src/iree/compiler/Codegen/Common/OptimizeTensorInsertExtractSlices.cpp
Outdated
Show resolved
Hide resolved
compiler/src/iree/compiler/Codegen/Common/OptimizeTensorInsertExtractSlices.cpp
Outdated
Show resolved
Hide resolved
compiler/src/iree/compiler/Codegen/Common/OptimizeTensorInsertExtractSlices.cpp
Outdated
Show resolved
Hide resolved
| bool changed = true; | ||
| while (changed) { |
There was a problem hiding this comment.
Could we bound this loop? I think the tript count shouldn't be more than the number of users? I want to make sure this doesn't accidentally become an infinite loop.
There was a problem hiding this comment.
+1 to this. Lets add num_iters bounded by 10 or something that should be enough.
| changed = false; | ||
| // Get all subset extraction uses of this iter_arg and try to hoist them | ||
| // out of the loop. | ||
| for (Operation *op : loopLike.getRegionIterArgs()[idx].getUsers()) { |
There was a problem hiding this comment.
Do we ever need to revisit the already processed users?
There was a problem hiding this comment.
Potentially not, but for now this is a WAR and number of iter args are expected to be small. So maybe we can ignore this for now.
MaheshRavishankar
force-pushed
the
fix-optimize-0d-bug
branch
2 times, most recently
from
134d313 to
662d3cc
Compare
|
Can we merge this? I want it for some tests in Sharktank. |
|
I see it introduces a regression in the performance of SDXL. |
MaheshRavishankar
force-pushed
the
fix-optimize-0d-bug
branch
from
662d3cc to
cb27811
Compare
Signed-off-by: Groverkss <kunwarshaanjeetsingh.grover@amd.com>
Signed-off-by: MaheshRavishankar <mahesh.ravishankar@gmail.com>
Signed-off-by: MaheshRavishankar <mahesh.ravishankar@gmail.com>
MaheshRavishankar
force-pushed
the
fix-optimize-0d-bug
branch
from
cb27811 to
7d77832
Compare
Yeah, looking into it |
The loop here is iterating on arguments of a dead operation. This sometimes works if the operation decided to use the same memory for it's iter arguments, but is relying on undefined behavior. This patch restarts the check each time a new loop is created.
No tests for this one, because it sometimes works, depending on how the memory allocator allocates the operation.